Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,26 @@ if(NOT TARGET readerwriterqueue)
FetchContent_MakeAvailable(ReaderWriterQueue)
endif()

if (NOT TARGET farbot)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THANK YOU for this contribution and taking the time to write it up.

I have a couple of concerns, which maybe you can help me figure out.

First, I never liked how I downloaded concurrentqueue "secretly" for the users. I think a (maybe better?) approach would be the users providing their own queue, perhaps wrapped in some External Polymorphism package like you've done here.

Now that we have two options (farbot AND concurrentqueue) we are downloading even more dependencies "secretly" for the user. It feels a little dirty, and I don't know what we should do about it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps one solution to this is to pass in the queue that the suer wants, but just provide two back ends that can be hidden behind some optional cmake

RTSAN_BUILD_FARBOT_BACKEND
RTSAN_BUILD_CONCURRENTQUEUE_BACKEND

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm completely spitballing here. I just want to make sure before we extend to 2 backends that we have a plan for when we inevitably need to add a 3rd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your concern. Once thing I would point out is that we would probably like to have those options alongside and not exclusive - one would like to have both kinds of loggers (also to avoid possible build config conflicts between sub-projects).

(BTW I didn't replace readerwriterqueue with concurrentqueue, so only farbot version is added. I can add that too if you want, but it's not going to be RT log anymore ;-) )

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm digging into this just a little bit and I remember one of the sticking points that prevented me from doing this in the past.

If you change the args to have a QueueType:

template <typename LogData, size_t MaxNumMessages, size_t MaxMessageLength,
          std::atomic<std::size_t>& SequenceNumber, typename QueueType>

This becomes tricky because QueueType needs to depend on InternalLogData.

(I'm not sure immediately how to re-arrange to get around this problem)

include(FetchContent)

FetchContent_Declare(farbot
GIT_REPOSITORY https://github.com/hogliux/farbot
GIT_TAG 0416705394720c12f0d02e55c144e4f69bb06912
)
# Note we do not "MakeAvailable" here, because farbot does not fully work via FetchContent
if(NOT farbot_POPULATED)
FetchContent_Populate(farbot)
endif()
add_library(farbot INTERFACE)
add_library(farbot::farbot ALIAS farbot)

target_include_directories(farbot INTERFACE
$<BUILD_INTERFACE:${farbot_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>
)
endif()

if(NOT TARGET stb::stb)
# Avoid warning about DOWNLOAD_EXTRACT_TIMESTAMP in CMake 3.24:
if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.24.0")
Expand Down Expand Up @@ -81,6 +101,7 @@ target_link_libraries(rtlog
INTERFACE
readerwriterqueue
stb::stb
farbot::farbot
$<$<BOOL:${RTLOG_USE_FMTLIB}>:fmt::fmt>
)

Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,14 @@ The design behind this logger was presented at ADCx 2023. Presentation [video](h
- Ability to log messages of any type and size from the real-time thread
- Statically allocated memory at compile time, no allocations in the real-time thread
- Support for printf-style format specifiers (using [a version of the printf family](https://github.com/nothings/stb/blob/master/stb_sprintf.h) that doesn't hit the `localeconv` lock)
- Efficient thread-safe logging using a [lock free queue](https://github.com/cameron314/readerwriterqueue).
- Efficient thread-safe logging using lock free queues (either [single consumer](https://github.com/cameron314/readerwriterqueue) or [multi consumer](https://github.com/hogliux/farbot)).

## Requirements

- A C++17 compatible compiler
- The C++17 standard library
- moodycamel::ReaderWriterQueue (will be downloaded via cmake if not provided)
- farbot's fifo (will be downloaded via cmake if not provided)
- stb's vsnprintf (will be downloaded via cmake if not provided)

## Installation via CMake
Expand Down
2 changes: 1 addition & 1 deletion examples/everlog/everlogmain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

namespace evr {
constexpr auto MAX_LOG_MESSAGE_LENGTH = 256;
constexpr auto MAX_NUM_LOG_MESSAGES = 100;
constexpr auto MAX_NUM_LOG_MESSAGES = 128;

enum class LogLevel { Debug, Info, Warning, Critical };

Expand Down
72 changes: 66 additions & 6 deletions include/rtlog/rtlog.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <fmt/format.h>
#endif // RTLOG_USE_FMTLIB

#include <farbot/fifo.hpp>
#include <readerwriterqueue.h>

#ifndef STB_SPRINTF_IMPLEMENTATION
Expand Down Expand Up @@ -47,14 +48,20 @@ enum class Status {
Error_MessageTruncated = 2,
};

enum class QueueConcurrency {
Single_Producer_Single_Consumer = 0,
Multi_Producer_Single_Consumer = 1,
};

/**
* @brief A logger class for logging messages.
* This class allows you to log messages of type LogData.
* This type is user defined, and is often the additional data outside the
* format string you want to log. For instance: The log level, the log region,
* the file name, the line number, etc. See examples or tests for some ideas.
*
* TODO: Currently is built on a single input/single output queue. Do not call
* NOTE: by default, it is built on a single input/single output queue. You have
* to specify QueueConcurrency for other types of queues. Otherwise, do not call
* Log or PrintAndClearLogQueue from multiple threads.
*
* @tparam LogData The type of the data to be logged.
Expand All @@ -65,9 +72,16 @@ enum class Status {
* @tparam SequenceNumber This number is incremented when the message is
* enqueued. It is assumed that your non-realtime logger increments and logs it
* on Log.
* @tparam QueueConcurrency The concurrency type of the internal queue.
* The default Single_Producer_Single_Consumer is for the simplest queue that
* works in single-producer thread model.
* Multi_Producer_Single_Consumer is for such an application that needs to
* handle multiple logging clients.
*/
template <typename LogData, size_t MaxNumMessages, size_t MaxMessageLength,
std::atomic<std::size_t> &SequenceNumber>
std::atomic<std::size_t> &SequenceNumber,
QueueConcurrency Concurrency =
QueueConcurrency::Single_Producer_Single_Consumer>
class Logger {
public:
/*
Expand Down Expand Up @@ -114,7 +128,7 @@ class Logger {

// Even if the message was truncated, we still try to enqueue it to minimize
// data loss
const bool dataWasEnqueued = mQueue.try_enqueue(dataToQueue);
const bool dataWasEnqueued = mQueue->tryEnqueue(std::move(dataToQueue));

if (!dataWasEnqueued)
retVal = Status::Error_QueueFull;
Expand Down Expand Up @@ -210,7 +224,7 @@ class Logger {

// Even if the message was truncated, we still try to enqueue it to minimize
// data loss
const bool dataWasEnqueued = mQueue.try_enqueue(dataToQueue);
const bool dataWasEnqueued = mQueue->tryEnqueue(std::move(dataToQueue));

if (!dataWasEnqueued)
retVal = Status::Error_QueueFull;
Expand Down Expand Up @@ -241,7 +255,7 @@ class Logger {
int numProcessed = 0;

InternalLogData value;
while (mQueue.try_dequeue(value)) {
while (mQueue->tryDequeue(value)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My other slight concern is this use of dynamic memory here. While it is perfectly real-time safe, it is also slower (probably) than the one that holds the queue on the stack.

Is there any way to have this be template based instead of inheritance based?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. It is probably possible. We would still need those wrapper classes to make code consistent though (yet removing inheritance).

printLogFn(value.mLogData, value.mSequenceNumber, "%s",
value.mMessage.data());
numProcessed++;
Expand All @@ -257,7 +271,53 @@ class Logger {
std::array<char, MaxMessageLength> mMessage{};
};

moodycamel::ReaderWriterQueue<InternalLogData> mQueue{MaxNumMessages};
class InternalQueue {
public:
virtual ~InternalQueue() = default;
virtual bool tryEnqueue(InternalLogData &&value) = 0;
virtual bool tryDequeue(InternalLogData &value) = 0;
};
class InternalQueueSPSC : public InternalQueue {
moodycamel::ReaderWriterQueue<InternalLogData> mQueue{MaxNumMessages};

public:
bool tryEnqueue(InternalLogData &&value) override {
return mQueue.try_enqueue(std::move(value));
}
bool tryDequeue(InternalLogData &value) override {
return mQueue.try_dequeue(value);
}
};
class InternalQueueMPSC : public InternalQueue {
farbot::fifo<InternalLogData, farbot::fifo_options::concurrency::single,
farbot::fifo_options::concurrency::multiple,
farbot::fifo_options::full_empty_failure_mode::
return_false_on_full_or_empty,
farbot::fifo_options::full_empty_failure_mode::
overwrite_or_return_default>
mQueue{MaxNumMessages};

public:
InternalQueueMPSC() {
static_assert((MaxNumMessages & (MaxNumMessages - 1)) == 0 ||
Concurrency !=
QueueConcurrency::Multi_Producer_Single_Consumer,
"you have to assign 2^n to MaxNumMessages (farbot backend "
"restriction)");
}
bool tryEnqueue(InternalLogData &&value) override {
return mQueue.push(std::move(value));
}
bool tryDequeue(InternalLogData &value) override {
return mQueue.pop(value);
}
};

std::unique_ptr<InternalQueue> mQueue{
Concurrency == QueueConcurrency::Single_Producer_Single_Consumer
? (std::unique_ptr<InternalQueue>)
std::make_unique<InternalQueueSPSC>()
: std::make_unique<InternalQueueMPSC>()};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

perhaps a case statement would be more clear and more extensible for the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed!

};

/**
Expand Down
18 changes: 17 additions & 1 deletion test/test_rtlog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace rtlog::test {
static std::atomic<std::size_t> gSequenceNumber{0};

constexpr auto MAX_LOG_MESSAGE_LENGTH = 256;
constexpr auto MAX_NUM_LOG_MESSAGES = 100;
constexpr auto MAX_NUM_LOG_MESSAGES = 128;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, same as maxNumMessages.


enum class ExampleLogLevel { Debug, Info, Warning, Critical };

Expand Down Expand Up @@ -82,6 +82,22 @@ TEST(RtlogTest, BasicConstruction) {
EXPECT_EQ(logger.PrintAndClearLogQueue(PrintMessage), 4);
}

TEST(RtlogTest, MPSCWorksAsIntended) {
rtlog::Logger<ExampleLogData, MAX_NUM_LOG_MESSAGES, MAX_LOG_MESSAGE_LENGTH,
gSequenceNumber,
rtlog::QueueConcurrency::Multi_Producer_Single_Consumer>
logger;
logger.Log({ExampleLogLevel::Debug, ExampleLogRegion::Engine},
"Hello, world!");
logger.Log({ExampleLogLevel::Info, ExampleLogRegion::Game}, "Hello, world!");
logger.Log({ExampleLogLevel::Warning, ExampleLogRegion::Network},
"Hello, world!");
logger.Log({ExampleLogLevel::Critical, ExampleLogRegion::Audio},
"Hello, world!");

EXPECT_EQ(logger.PrintAndClearLogQueue(PrintMessage), 4);
}

TEST(RtlogTest, VaArgsWorksAsIntended) {
rtlog::Logger<ExampleLogData, MAX_NUM_LOG_MESSAGES, MAX_LOG_MESSAGE_LENGTH,
gSequenceNumber>
Expand Down
Loading