Skip to content

Commit db4bde0

Browse files
committed
Merge bitcoin/bitcoin#33517: multiprocess: Fix high overhead from message logging
0626b90 multiprocess: align our logging with libmultiprocess's (Cory Fields) 9d06822 multiprocess: update multiprocess EventLoop construction to use options (Cory Fields) Pull request description: This fixes bitcoin-core/libmultiprocess#215 on Core's side. ~It depends on bitcoin-core/libmultiprocess#220 being merged upstream, and a PR to update our subtree. I've included a subtree merge from my repo here for now, but will rebase on top of the merge from upstream once it's in.~ Edit: Rebased on top of #33518. For context: before bitcoin-core/libmultiprocess#220, libmultiprocess serializesd every log message parameter, even if that message was ultimately going to be discarded. The upstream PR accomplished 2 main things: - Creates logging categories, similar to Core's - Using macros, avoids serializing parameters for disabled log levels. That allowed the expensive serialization to be skipped, but the default log level is `Trace`. This PR updates our usage of libmultiprocess options to honor our log categories and levels. Because of the substantial unnecessary overhead (see the [flamegraphs](bitcoin-core/libmultiprocess#215). Logging accounts for 50% of my application's cpu time, and nearly 10% of bitcoin-node's, both of which go to ~0% once fixed), it'd be a shame to ship the first multiprocess binaries without this fixed. So I propose that we also backport this (and the required libmultiprocess subtree merge) ~to v30. Sorry about the timing~ :( Edit: Didn't make it for v30, but it would still make sense to backport for a v30.1. ACKs for top commit: Sjors: ACK 0626b90 TheCharlatan: ACK 0626b90 sipa: utACK 0626b90 Tree-SHA512: 70b63b62d1f6de547f4d4775538d7bcaf32f57d8a72c5b26762b57755810c8be6942d9dfebab43cf1c1d8d025a555f72a48e9ebf3d84f8d40d6592ca801cda5d
2 parents 9314113 + 0626b90 commit db4bde0

File tree

1 file changed

+39
-5
lines changed

1 file changed

+39
-5
lines changed

src/ipc/capnp/protocol.cpp

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,36 @@
3030
namespace ipc {
3131
namespace capnp {
3232
namespace {
33-
void IpcLogFn(bool raise, std::string message)
33+
34+
BCLog::Level ConvertIPCLogLevel(mp::Log level)
35+
{
36+
switch (level) {
37+
case mp::Log::Trace: return BCLog::Level::Trace;
38+
case mp::Log::Debug: return BCLog::Level::Debug;
39+
case mp::Log::Info: return BCLog::Level::Info;
40+
case mp::Log::Warning: return BCLog::Level::Warning;
41+
case mp::Log::Error: return BCLog::Level::Error;
42+
case mp::Log::Raise: return BCLog::Level::Error;
43+
} // no default case, so the compiler can warn about missing cases
44+
45+
// Be conservative and assume that if MP ever adds a new log level, it
46+
// should only be shown at our most verbose level.
47+
return BCLog::Level::Trace;
48+
}
49+
50+
mp::Log GetRequestedIPCLogLevel()
51+
{
52+
if (LogAcceptCategory(BCLog::IPC, BCLog::Level::Trace)) return mp::Log::Trace;
53+
if (LogAcceptCategory(BCLog::IPC, BCLog::Level::Debug)) return mp::Log::Debug;
54+
55+
// Info, Warning, and Error are logged unconditionally
56+
return mp::Log::Info;
57+
}
58+
59+
void IpcLogFn(mp::LogMessage message)
3460
{
35-
LogDebug(BCLog::IPC, "%s\n", message);
36-
if (raise) throw Exception(message);
61+
LogPrintLevel(BCLog::IPC, ConvertIPCLogLevel(message.level), "%s\n", message.message);
62+
if (message.level == mp::Log::Raise) throw Exception(message.message);
3763
}
3864

3965
class CapnpProtocol : public Protocol
@@ -62,7 +88,11 @@ class CapnpProtocol : public Protocol
6288
{
6389
assert(!m_loop);
6490
mp::g_thread_context.thread_name = mp::ThreadName(exe_name);
65-
m_loop.emplace(exe_name, &IpcLogFn, &m_context);
91+
mp::LogOptions opts = {
92+
.log_fn = IpcLogFn,
93+
.log_level = GetRequestedIPCLogLevel()
94+
};
95+
m_loop.emplace(exe_name, std::move(opts), &m_context);
6696
if (ready_fn) ready_fn();
6797
mp::ServeStream<messages::Init>(*m_loop, fd, init);
6898
m_parent_connection = &m_loop->m_incoming_connections.back();
@@ -90,7 +120,11 @@ class CapnpProtocol : public Protocol
90120
std::promise<void> promise;
91121
m_loop_thread = std::thread([&] {
92122
util::ThreadRename("capnp-loop");
93-
m_loop.emplace(exe_name, &IpcLogFn, &m_context);
123+
mp::LogOptions opts = {
124+
.log_fn = IpcLogFn,
125+
.log_level = GetRequestedIPCLogLevel()
126+
};
127+
m_loop.emplace(exe_name, std::move(opts), &m_context);
94128
m_loop_ref.emplace(*m_loop);
95129
promise.set_value();
96130
m_loop->loop();

0 commit comments

Comments
 (0)