Increase the maximum number of logging categories from 32 to 64.
We’re currently using 29 of the 32 available logging categories (there are only 3 remaining). It would be good to increase the limit soon; the fourth PR to be merged that adds a new logging category will be blocked until something like this is done.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | vasild, ryanofsky, theStack, achow101 |
| Approach ACK | stickies-v |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
181@@ -182,6 +182,9 @@ const CLogCategoryDesc LogCategories[] =
182 {BCLog::BLOCKSTORE, "blockstorage"},
183 {BCLog::TXRECONCILIATION, "txreconciliation"},
184 {BCLog::SCAN, "scan"},
185+ // Add new entries before this line.
186+
187+ {BCLog::TEST, "test"}, // logging testing only
0 {BCLog::TEST, "test"}, // internal flag only to be used to test logging
LOGTEST instead of TEST? Makes the purpose more clear and also keeps the TEST name free if we want to use it later?
Concept ACK - makes sense to expand this now
There are still a couple of places that use uint32_t, e.g. here https://github.com/bitcoin/bitcoin/blob/02515117dc5a25998f5dd92938eccb326b1eec5c/src/logging.h#L174 and callsites of GetCategoryMask() (and possibly further upstream)
Force-pushed to address review comments – thanks, @stickies-v!
One further change this PR could make is to modify all the existing BCLog::LogFlags definitions as follows:
0- NET = (1 << 0),
1+ NET = (1ULL << 0),
More lines would change, but greater consistency. Any opinions?
More lines would change, but greater consistency. Any opinions?
I wouldn’t, there’s not really a connection with this PR and the lines of code aren’t touched.
Approach ACK fe99f6858
I can’t make up my mind if I think the LOGTEST flag is helpful. PR states it helps unearth wrong 32bits assumptions, but… we only use it in a single function call (LogPrint) - so, does it really? Atm I’m leaning towards preferring leaving it out.
Otherwise, everything looks good to me with some basic testing.
What’s the benefit from growing it prematurely?
Maybe there should be a typedef? (Been thinking we should have a blockheight_t or such too, but no real benefit to the refactor)
What’s the benefit from growing it prematurely?
If we have multiple PRs open that each need to add their own new log category, it would be unclear which PR is to eventually exceed the limit and/or multiple PRs would have to implement this upgrade independently and then rebase. It makes sense to me do to this in advance, and I think we’re close enough to the limit to go ahead? Could probably wait for another category to be added first too, but… since it’s already implemented and we’ll need it soon anyway, I think it’s best to not waste more review/dev time by letting this go stale.
Maybe there should be a typedef?
That seems sensible. I don’t think we’ll need to replace the uint64_t anytime soon, but we’re changing all the types now so might as well include that.
but we’re changing all the types now so might as well include that
Just so I understand, is the suggestion (for this PR) to add a typedef rather than using uint64_t directly? I’d be happy to do that, and I think you’re right, we can keep this PR smaller by eliminating the test stuff, I could do that as well. Please give this comment a thumbs up (or down) to let me know, thanks.
Force-pushed to remove the test; I agree it doesn’t add enough value for the additional complexity.
Maybe there should be a typedef?
I don’t think we’ll need to replace the uint64_t anytime soon, but we’re changing all the types now so might as well include that.
I’m now leaning toward keeping this patch as simple as possible, just the straightforward change from uint32_t to uint64_t. If we introduce a new typedef, then if we ever need more than 64 logging categories, we won’t be able to just change that one typedef because there’s no standard uint128_t, so we’d need bigger changes anyway.
If we introduce a new typedef, then if we ever need more than 64 logging categories, we won’t be able to just change that one typedef because there’s no standard uint128_t, so we’d need bigger changes anyway.
We’d be able to introduce a class and change the typedef to that. Perhaps similar to QFlags.
But actually, it doesn’t really make sense for LogFlags to be allocated by bit at all. The only time we need to indicate multiple is the configuration for logging them, right? That could easily be a std::bitset.
But actually, it doesn’t really make sense for LogFlags to be allocated by bit at all. The only time we need to indicate multiple is the configuration for logging them, right? That could easily be a
std::bitset.
That’s an excellent suggestion; std::bitset looks perfect for this. Would you favor changing this PR to implement that? Or maybe do this simple one now, and then do a follow-up?
I think there’s no urgent need for >32 bits, so might as well go straight to bitset. Perhaps close this and open a new PR for it, to avoid confusion?
I just created #26697 as a replacement for this PR, closing. If someone can think of a reason this PR would be better than #26697, leave a comment here and I’ll reopen it.
enum LogFlags literals’ type match the enum type, uint64_t (instead of uint32_t). This isn’t necessary unless the shift amount is 32 or more, but this way all these constants will be defined consistently.
ACK 90c3d904bcbf294a6fcef8bc9ecb9445fd41c4bd
This probably needs a rebase. It is a trivial change.
10 new categories were added in the last 7 years. That is 1-2 per year. 32 extra should suffice for a few decades.
203@@ -204,7 +204,7 @@ namespace BCLog {
204 void SetLogLevel(Level level) { m_log_level = level; }
205 bool SetLogLevel(std::string_view level);
206
207- uint32_t GetCategoryMask() const { return m_categories.load(); }
208+ uint64_t GetCategoryMask() const { return m_categories.load(); }
In commit “log: expand BCLog::LogFlags (categories) to 64 bits” (9bed303b9cc28e13b12af067b34e6058cbccf639)
Instead of hardcoding uint64_t throughout this PR, it might make sense to add using CategoryMask = uint64_t; and use CategoryMask as a type name describing purpose of the type rather than how many bits it presently contains.
Instead of hardcoding uint64_t throughout this PR, it might make sense to add using CategoryMask = uint64_t
I took this suggestion, thanks. As a test, I defined CategoryMask to be uint32_t, but the compile failed because of the ULL suffix on all those constants (width exceeds 32 bits). So I’m trying to work around that by defining a mask_bit constant. (Should it have a name like MaskBit? It’s only used within the enum definition.) I think it makes the code easy to read. If CI fails, I can change them back to ULL (even though it’s kind of old fashioned and ugly).
ACK f64b861c0940ca90869303a9754ebe5bf40bb0c7
I was fine with (1ULL << 23) as well. Another way is to use the 0b prefix (then no need for the ULL suffix):
0enum LogFlags : CategoryMask {
1 NONE = 0b00000000000000000000000000000000,
2 NET = 0b00000000000000000000000000000001,
3 TOR = 0b00000000000000000000000000000010,
4 MEMPOOL = 0b00000000000000000000000000000100,
5 HTTP = 0b00000000000000000000000000001000,
6 ....
7};
Code review ACK f64b861c0940ca90869303a9754ebe5bf40bb0c7, just added uint64_t typedef since last review.
As a test, I defined
CategoryMaskto beuint32_t, but the compile failed because of theULLsuffix on all those constants (width exceeds 32 bits). So I’m trying to work around that by defining amask_bitconstant. (Should it have a name likeMaskBit? It’s only used within theenumdefinition.) I think it makes the code easy to read. If CI fails, I can change them back toULL(even though it’s kind of old fashioned and ugly).
This seems ok, though I think would just write CategoryMask{1} everywhere instead of having a separate constant. Since the code is already using CategoryMask{0} this seems consistent, and it avoids defining another identifier to look up. I do think either of these approaches are a little better than using ULL suffixes, but those suffixes would be ok too. I have to say I am horrified :scream: by vasild’s suggestion to write the flags in binary because its seems dizzying and error prone to me, but maybe this representation seems more natural to other people.
I also still think #26697 is a nicer alternative to this, removing hardcoded numbers and bitwise operations outside of the bitset class.
0b00000000'00000000'00000000'00001000 is a bit less horrific ;) The current is ok, no need to change it further.
This will increase the maximum number of logging categories
from 32 to 64.
I think would just write
CategoryMask{1}everywhere instead of having a separate constant
I like that suggestion, I just force-pushed it, thanks!
