From f150b361084be6d7e0e66618bee51608552967ac Mon Sep 17 00:00:00 2001 From: Gold856 <117957790+Gold856@users.noreply.github.com> Date: Tue, 8 Oct 2024 14:55:16 -0400 Subject: [PATCH] [wpiutil] Fix FileLogger behavior and performance (#7150) Co-authored-by: Ryan Blue --- wpiutil/src/main/native/cpp/FileLogger.cpp | 41 +++++++++---------- .../src/main/native/include/wpi/FileLogger.h | 8 ++-- .../src/test/native/cpp/FileLoggerTest.cpp | 40 ++++++++++-------- 3 files changed, 47 insertions(+), 42 deletions(-) diff --git a/wpiutil/src/main/native/cpp/FileLogger.cpp b/wpiutil/src/main/native/cpp/FileLogger.cpp index 6e1435673b4..70ee9f95ec3 100644 --- a/wpiutil/src/main/native/cpp/FileLogger.cpp +++ b/wpiutil/src/main/native/cpp/FileLogger.cpp @@ -10,11 +10,15 @@ #include #endif +#include #include #include +#include #include #include +#include + #include "wpi/StringExtras.h" namespace wpi { @@ -26,15 +30,15 @@ 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(bufLen)}); } + std::this_thread::sleep_for(std::chrono::milliseconds(100)); } }} #endif @@ -42,8 +46,8 @@ FileLogger::FileLogger(std::string_view file, } 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 FileLogger::LineBuffer( + +std::function FileLogger::Buffer( std::function callback) { return [callback, - buf = wpi::SmallVector{}](std::string_view data) mutable { - if (!wpi::contains(data, "\n")) { - buf.append(data.begin(), data.end()); + buf = wpi::SmallVector{}](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 index 72565ebc804..c04cf5b7edc 100644 --- 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 LineBuffer( + static std::function Buffer( std::function callback); private: diff --git a/wpiutil/src/test/native/cpp/FileLoggerTest.cpp b/wpiutil/src/test/native/cpp/FileLoggerTest.cpp index 0fbb54f160e..f402ef583dc 100644 --- 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 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 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 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 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 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]); }