Я сделал многопоточный регистратор много лет назад, когда был новичком. «Сработало» (когда не зашло в тупик). Теперь, когда я немного меньше новичок, я хотел бы получить некоторую критику по поводу этой новой версии.
Регистратор порождает внутренний поток своим собственным методом writer()
, целью которого является запись (в консоль и файл) информации журнала.
Есть две очереди, одна из которых записывает данные (queue_write
), другой — где остальная часть программы может передавать данные (queue_log
). При нажатии писатель пробуждается, он берет на себя управление мьютексом, чтобы убедиться, что никакой другой поток не обращается к нему. queue_log
очередь, и он меняет местами содержимое с queue_write
; затем снимает блокировку. Таким образом, потокам, использующим регистратор, в худшем случае придется ждать простой замены указателя между двумя очередями, что должно быть эффективным.
После свопа поток записи очищает свою новую очередь в выходных потоках до тех пор, пока она не станет пустой, затем переходит к проверке, работает ли регистратор по-прежнему (ложно при разрушении или явном закрытии), снова берет блокировку, и если queue_log
пусто, он засыпает, отпуская замок.
Пара замечаний:
- «
utils
«namespace — это пространство имен утилит, которые я создал для себя и охотно отражает реализацию STL. Выбор начала класса в нижнем регистре сделан специально. - Регистратор выполнен с одной резьбой, производящей и потребляющей внутреннюю резьбу; он не предназначен для нескольких потребителей. У него может быть несколько производителей, но он не оптимизирован с учетом этого.
- В
Message
класс, который я использую в примере использования, лишен всего нерелевантного контента (у меня есть другая частьutils
заботясь о раскрашивании вывода на консоль).
logger.h
#pragma once
#include <fstream>
#include <iostream>
#include <queue>
#include <thread>
#include <mutex>
namespace utils
{
//TODO
// With C++20 replace the time_points with actual readable time https://stackoverflow.com/questions/62954221/c-print-chronotime-pointchronohigh-resolution-clock-to-be-readable
template <typename Message_type>
class logger
{
private:
std::ofstream file;
std::queue<Message_type> queue_log;
std::queue<Message_type> queue_write;
std::thread thread;
std::atomic_bool running = true;
std::mutex queue_free;
std::condition_variable work_available;
void writer()
{
while (running)
{
if (true)
{
std::unique_lock lock{queue_free};
if (queue_log.empty()) { work_available.wait(lock); }
queue_write.swap(queue_log);
}
write_all();
}
}
void write_all()
{
while (!queue_write.empty())
{
Message_type& message = queue_write.front();
std::cout << message << std::endl;
file << message << std::endl;
queue_write.pop();
}
}
public:
logger(const std::string& file_name = "log.txt")
: thread(&logger::writer, this) { file.open(file_name); }
logger(const logger& copy) = delete; // don't copy threads please, thank you
logger& operator=(const logger& copy) = delete; // don't copy threads please, thank you
logger(logger&& move) noexcept = default; //could make those (running is already a flag for the instance being alive or not) but I'm lazy
logger& operator=(logger&& move) = default; //could make those (running is already a flag for the instance being alive or not) but I'm lazy
~logger() { if (running) { close(); } }
//Push messages begin
void operator<<(const Message_type& message) { push(message); }
void operator()(const Message_type& message) { push(message); }
template <typename ...Args>
void emplace(Args&&... args)
{
Message_type message{args...};
push(message);
}
void push(const Message_type& message)
{
if (true)
{
std::lock_guard lock(queue_free);
queue_log.push(message);
}
work_available.notify_one();
}
//Push messages end
void close()
{
running = false;
work_available.notify_one();
thread.join();
queue_write.swap(queue_log);
write_all();
file.close();
}
};
}
Пример использования
Message.h
#pragma once
#include <chrono>
#include <iomanip>
#include <string>
#include <string_view>
class Message
{
private:
static std::string filter_last_newline(const std::string& string)
{
if (string.length() > 0 && string[string.length() - 1] == 'n') { return string.substr(0, string.length() - 1); }
else { return string; }
}
public:
enum class Type { log, dgn, inf, wrn, err };
Message(Type type, const std::string& string, std::chrono::time_point<std::chrono::system_clock> time) noexcept
: type(type), string(string), time(time)
{}
Message(Type type, const std::string& string) noexcept
: type(type), string(string), time(std::chrono::system_clock::now())
{}
Message(const std::string& string) noexcept
: type(Type::log), string(string), time(std::chrono::system_clock::now())
{}
Type type = Type::log;
std::string string = "";
std::chrono::time_point<std::chrono::system_clock> time;
const char* out_type() const noexcept
{
switch (type)
{
case Message::Type::log: return "[LOG]"; // [LOG]
case Message::Type::dgn: return "[DGN]"; // [DIAGNOSTIC]
case Message::Type::inf: return "[INF]"; // [INFO]
case Message::Type::wrn: return "[WRN]"; // [WARNING]
case Message::Type::err: return "[ERR]"; // [ERROR]
}
}
static Message log(const std::string& string) { return {Type::log, filter_last_newline(string), std::chrono::system_clock::now()}; }
static Message dgn(const std::string& string) { return {Type::dgn, filter_last_newline(string), std::chrono::system_clock::now()}; }
static Message inf(const std::string& string) { return {Type::inf, filter_last_newline(string), std::chrono::system_clock::now()}; }
static Message wrn(const std::string& string) { return {Type::wrn, filter_last_newline(string), std::chrono::system_clock::now()}; }
static Message err(const std::string& string) { return {Type::err, filter_last_newline(string), std::chrono::system_clock::now()}; }
friend std::ostream& operator<<(std::ostream& os, const Message& m)
{
size_t beg = 0;
size_t end = m.string.find_first_of('n', beg);
if (end == std::string::npos) { end = m.string.length(); }
//First line
os << std::setw(18) << m.time.time_since_epoch().count() << ' ';
os << m.out_type();
os << ' ' << std::string_view(m.string).substr(beg, end - beg) << 'n';
//Other lines
while (true)
{
if (end == m.string.length()) { break; }
else
{
beg = end + 1;
end = m.string.find_first_of('n', beg);
if (end == std::string::npos) { end = m.string.length(); }
}
os << std::setw(24) << '|';
os << ' ' << std::string_view(m.string).substr(beg, end - beg) << 'n';
}
return os;
}
};
Main.cpp
#include "utils/logger.h"
#include "Message.h"
int main()
{
utils::logger<Message> logger;
logger.push(Message(Message::Type::err, "This is a verbose way to write an error."));
logger.emplace(Message::Type::err, "This is a still quite verbose to write an error.");
logger.push(Message::err("This is a less verbose way to write an error."));
logger << Message::err("This is the slightly shorter way to write an error.");
logger(Message::err("This is the shortest I could think of, although far from the "standard" way of doing things."));
logger(Message::wrn("Multinlinenwarningnjustntonmakensurenitnworks."));
return 0;
}
1 ответ
Хотя форматирование является делом ваших предпочтений, вы не должны отходить слишком далеко от общих оснований. Подумайте о том, чтобы переместить открывающую и закрывающую скобки назад, чтобы:
int main()
{
return 0;
}
вместо
int main()
{
return 0;
}
Message_type
Сделайте стойку, CamelCase или snake_case, а не какое-нибудь диковинное детище.
while (running)
{
if (true)
if (true)
здесь избыточно, вы можете использовать { .. }
Только.
if (queue_log.empty()) { work_available.wait(lock); }
вместо этого идите:
work_available.wait(lock, [&]() { return !running || !queue_log.empty(); });
std::condition_variable::wait()
может ложно проснуться (без причины), и в этом случае ваш queue_log
будет пусто, и вы позвонили write_all
зря. wait
call с предикатом позаботится об этом.
Message_type& message = queue_write.front();
использовать auto
:
auto& message = queue_write.front();
это упростит рефакторинг кода.
while (!queue_write.empty())
{
Message_type& message = queue_write.front();
std::cout << message << std::endl;
file << message << std::endl;
queue_write.pop();
}
Использовать std::vector
вместо std::queue
— вам не нужно выскакивать из queue_write
, вы можете просто перебрать его и уничтожить все вместе. Так:
for(auto &message : queue_write) {
std::cout << message << std::endl;
file << message << std::endl;
}
и:
std::vector<Message_type> queue_log;
std::vector<Message_type> queue_write;
вместо:
std::queue<Message_type> queue_log;
std::queue<Message_type> queue_write;
Проходить queue_write
аргументом. Не храните его как поле участника. Передача его в качестве аргумента показывает, что значение является «частным» для вызывающего и вызываемого. Когда вы помещаете его как поле-член, каждая функция-член может получить к нему доступ, поэтому кому-либо еще придется сканировать больше исходного кода, чтобы убедиться, что к нему обращаются только в двух функциях. Так:
void write_all(const std::vector<Message_type> &queue_write) {
for(auto &message: queue_write) {
std::cout << message << std::endl;
file << message << std::endl;
}
}
и
void writer() {
while (running) {
std::vector<Message_type> queue_write;
{
std::unique_lock lock{queue_free};
if (queue_log.empty()) { work_available.wait(lock); }
queue_write.swap(queue_write);
}
write_all(queue_write);
}
}
logger(const std::string& file_name = "log.txt")
: thread(&logger::writer, this) { file.open(file_name); }
это неопределенное поведение (состояние гонки) и серьезная ошибка. Вы создаете поток, который использует file
переменной, а также использует эту переменную в конструкторе, который находится на разные нить. Вместо этого напишите это так:
logger(const std::string& file_name = "log.txt")
: file(file_name), thread(&logger::writer, this) {}
file
переменная перед thread
переменная, что означает, что она будет инициализирована первой. Таким образом вы непременно создадите поток после открытие вашего файла для записи.
void operator<<(const Message_type& message) { push(message); }
void operator()(const Message_type& message) { push(message); }
вместо этого напишите:
void operator<<(Message_type message) { push(std::move(message)); }
void operator()(Message_type message) { push(std::move(message)); }
Это может помешать копированию message
.
void push(const Message_type& message)
вместо этого используйте:
void push(Message_type message)
{
if (true)
{
std::lock_guard lock(queue_free);
queue_log.push(std::move(message));
}
work_available.notify_one();
}
Если вы беспокоитесь о Message_type
будучи неподвижным, то предоставьте оба const Message_type &
и Message_type &&
перегрузки.
static std::string filter_last_newline(const std::string& string)
{
if (string.length() > 0 && string[string.length() - 1] == 'n') { return string.substr(0, string.length() - 1); }
else { return string; }
}
вместо этого напишите:
static std::string filter_last_newline(std::string str) {
if (!str.empty() && str.back() == 'n')
str.pop_back();
// c++17 rules about copy ellision are convoluted,
// i'm not sure, if compiler will be forced to do std::move here
// when returning value passed in as an argument, so just in case
// we std::move explicitly
return std::move(str);
}
Это может предотвратить ненужное копирование строки.
Message(Type type, const std::string& string, std::chrono::time_point<std::chrono::system_clock> time) noexcept
: type(type), string(string), time(time)
{}
Как правило, аргументы в конструкторах принимают по значению, а не по ссылке const, поэтому:
Message(Type type, std::string string, std::chrono::time_point<std::chrono::system_clock> time) noexcept
: type(type), string(std::move(string)), time(time)
{}
еще раз это может помешать копированию строки.
std::string string = "";
std::string
уже есть конструктор по умолчанию, поэтому = ""
избыточно.
switch (type)
{
case Message::Type::log: return "[LOG]"; // [LOG]
case Message::Type::dgn: return "[DGN]"; // [DIAGNOSTIC]
case Message::Type::inf: return "[INF]"; // [INFO]
case Message::Type::wrn: return "[WRN]"; // [WARNING]
case Message::Type::err: return "[ERR]"; // [ERROR]
}
в таком случае всегда добавляйте утверждение в конце функции, чтобы убедиться, что никто не доберется до нее. Этот одинарный лайнер может сэкономить вам много времени на отладку.
while (true)
{
if (end == m.string.length()) { break; }
вместо этого используйте while
условие:
while (end < m.string.length()) {
Как правило, журналы должны отображаться в порядке получения. Используя должным образом потокобезопасную очередь (упаковывая
std::queue
интерфейс сstd::scoped_lock
) это обрабатывается автоматически и легко понять. Используяstd::vector
не интуитивно понятен и потенциально сбивает с толку.— Кейси
«Сделай стойку, CamelCase или snake_case, а не какое-нибудь странное детище». camelCase и snake_case больше говорят о том, что отделяет первое слово от второго. Первая заглавная буква или нет, обычно используется для отделения экземпляров от классов. CamelCase и Snake_case будут классами, camelCase и snake_case — экземплярами. Если вы не используете префиксный путь «class_xxx», «obj_xxx» etcc. Похожая вещь для позиционирования подтяжек, я использую отступы Whitesmiths, которые, по общему признанию, не очень популярны. en.wikipedia.org/wiki/Indentation_style
— Барнак
Что меня больше всего волнует, так это то, чтобы интерфейс был максимально похож на stl. Тай для простой реализации filter_last_newline, я даже не знал, что есть метод pop для строк. Я обязательно внимательно рассмотрю все указанные вами оптимизации по предотвращению копирования, спасибо!
— Барнак