[wpiutil] Fix FileLogger behavior and performance (wpilibsuite#7150)

Co-authored-by: Ryan Blue <[email protected]>
rzblue · Oct 8, 2024 · f150b36 · f150b36
1 parent f856c05
commit f150b36
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 42 deletions.
diff --git a/wpiutil/src/main/native/cpp/FileLogger.cpp b/wpiutil/src/main/native/cpp/FileLogger.cpp
@@ -10,11 +10,15 @@
 #include <unistd.h>
 #endif
 
+#include <chrono>
 #include <string>
 #include <string_view>
+#include <thread>
 #include <tuple>
 #include <utility>
 
+#include <fmt/format.h>
+
 #include "wpi/StringExtras.h"
 
 namespace wpi {
@@ -26,24 +30,24 @@ FileLogger::FileLogger(std::string_view file,
       m_inotifyWatchHandle{
           inotify_add_watch(m_inotifyHandle, file.data(), IN_MODIFY)},
       m_thread{[=, this] {
-        char buf[4000];
-        struct inotify_event ev;
-        int len = 0;
+        char buf[8000];
+        char eventBuf[sizeof(struct inotify_event) + NAME_MAX + 1];
         lseek(m_fileHandle, 0, SEEK_END);
-        while ((len = read(m_inotifyHandle, &ev, sizeof(ev))) > 0) {
+        while (read(m_inotifyHandle, eventBuf, sizeof(eventBuf)) > 0) {
           int bufLen = 0;
-          if ((bufLen = read(m_fileHandle, buf, sizeof(buf)) > 0)) {
+          if ((bufLen = read(m_fileHandle, buf, sizeof(buf))) > 0) {
             callback(std::string_view{buf, static_cast<size_t>(bufLen)});
           }
+          std::this_thread::sleep_for(std::chrono::milliseconds(100));
         }
       }}
 #endif
 {
 }
 FileLogger::FileLogger(std::string_view file, log::DataLog& log,
                        std::string_view key)
-    : FileLogger(file, LineBuffer([entry = log.Start(key, "string"),
-                                   &log](std::string_view line) {
+    : FileLogger(file, Buffer([entry = log.Start(key, "string"),
+                               &log](std::string_view line) {
                    log.AppendString(entry, line, 0);
                  })) {}
 FileLogger::FileLogger(FileLogger&& other)
@@ -80,26 +84,21 @@ FileLogger::~FileLogger() {
   }
 #endif
 }
-std::function<void(std::string_view)> FileLogger::LineBuffer(
+
+std::function<void(std::string_view)> FileLogger::Buffer(
     std::function<void(std::string_view)> callback) {
   return [callback,
-          buf = wpi::SmallVector<char, 32>{}](std::string_view data) mutable {
-    if (!wpi::contains(data, "\n")) {
-      buf.append(data.begin(), data.end());
+          buf = wpi::SmallVector<char, 64>{}](std::string_view data) mutable {
+    buf.append(data.begin(), data.end());
+    if (!wpi::contains({data.data(), data.size()}, "\n")) {
       return;
     }
-    std::string_view line;
-    std::string_view remainingData;
-    std::tie(line, remainingData) = wpi::split(data, "\n");
-    buf.append(line.begin(), line.end());
-    callback(std::string_view{buf.data(), buf.size()});
+    auto [wholeData, extra] = wpi::rsplit({buf.data(), buf.size()}, "\n");
+    std::string leftover{extra};
 
-    while (wpi::contains(remainingData, "\n")) {
-      std::tie(line, remainingData) = wpi::split(remainingData, "\n");
-      callback(line);
-    }
+    callback(wholeData);
     buf.clear();
-    buf.append(remainingData.begin(), remainingData.end());
+    buf.append(leftover.begin(), leftover.end());
   };
 }
 }  // namespace wpi
diff --git a/wpiutil/src/main/native/include/wpi/FileLogger.h b/wpiutil/src/main/native/include/wpi/FileLogger.h
@@ -41,13 +41,13 @@ class FileLogger {
   FileLogger& operator=(FileLogger&& rhs);
   ~FileLogger();
   /**
-   * Creates a function that chunks incoming data into lines before calling the
-   * callback with the individual line.
+   * Creates a function that chunks incoming data into blocks of whole lines and
+   * stores incomplete lines to add to the next block of data.
    *
-   * @param callback The callback that logs lines.
+   * @param callback A callback that accepts the blocks of whole lines.
    * @return The function.
    */
-  static std::function<void(std::string_view)> LineBuffer(
+  static std::function<void(std::string_view)> Buffer(
       std::function<void(std::string_view)> callback);
 
  private:

diff --git a/wpiutil/src/test/native/cpp/FileLoggerTest.cpp b/wpiutil/src/test/native/cpp/FileLoggerTest.cpp
@@ -10,44 +10,50 @@
 
 #include "wpi/FileLogger.h"
 
-TEST(FileLoggerTest, LineBufferSingleLine) {
+TEST(FileLoggerTest, BufferSingleLine) {
   std::vector<std::string> buf;
-  auto func = wpi::FileLogger::LineBuffer(
+  auto func = wpi::FileLogger::Buffer(
       [&buf](std::string_view line) { buf.emplace_back(line); });
   func("qwertyuiop\n");
-  EXPECT_EQ(buf.front(), "qwertyuiop");
-  buf.clear();
+  EXPECT_EQ("qwertyuiop", buf[0]);
 }
 
-TEST(FileLoggerTest, LineBufferMultiLine) {
+TEST(FileLoggerTest, BufferMultiLine) {
   std::vector<std::string> buf;
-  auto func = wpi::FileLogger::LineBuffer(
+  auto func = wpi::FileLogger::Buffer(
       [&buf](std::string_view line) { buf.emplace_back(line); });
   func("line 1\nline 2\nline 3\n");
-  EXPECT_EQ("line 1", buf[0]);
-  EXPECT_EQ("line 2", buf[1]);
-  EXPECT_EQ("line 3", buf[2]);
+  EXPECT_EQ("line 1\nline 2\nline 3", buf[0]);
 }
 
-TEST(FileLoggerTest, LineBufferPartials) {
+TEST(FileLoggerTest, BufferPartials) {
   std::vector<std::string> buf;
-  auto func = wpi::FileLogger::LineBuffer(
+  auto func = wpi::FileLogger::Buffer(
       [&buf](std::string_view line) { buf.emplace_back(line); });
   func("part 1");
   func("part 2\npart 3");
-  EXPECT_EQ("part 1part 2", buf.front());
-  buf.clear();
+  EXPECT_EQ("part 1part 2", buf[0]);
   func("\n");
-  EXPECT_EQ("part 3", buf.front());
+  EXPECT_EQ("part 3", buf[1]);
 }
 
-TEST(FileLoggerTest, LineBufferMultiplePartials) {
+TEST(FileLoggerTest, BufferMultiplePartials) {
   std::vector<std::string> buf;
-  auto func = wpi::FileLogger::LineBuffer(
+  auto func = wpi::FileLogger::Buffer(
       [&buf](std::string_view line) { buf.emplace_back(line); });
   func("part 1");
   func("part 2");
   func("part 3");
   func("part 4\n");
-  EXPECT_EQ("part 1part 2part 3part 4", buf.front());
+  EXPECT_EQ("part 1part 2part 3part 4", buf[0]);
+}
+TEST(FileLoggerTest, BufferMultipleMultiLinePartials) {
+  std::vector<std::string> buf;
+  auto func = wpi::FileLogger::Buffer(
+      [&buf](std::string_view line) { buf.emplace_back(line); });
+  func("part 1");
+  func("part 2\npart 3");
+  func("part 4\n");
+  EXPECT_EQ("part 1part 2", buf[0]);
+  EXPECT_EQ("part 3part 4", buf[1]);
 }