log: expand BCLog::LogFlags (categories) to 64 bits #26619
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
There was a problem hiding this comment.
Concept ACK - makes sense to expand this now
There are still a couple of places that use uint32_t, e.g. here
Line 174 in 0251511
GetCategoryMask() (and possibly further upstream)
c91cfa8 to
fe99f68
Compare
|
Force-pushed to address review comments -- thanks, @stickies-v! One further change this PR could make is to modify all the existing - NET = (1 << 0),
+ NET = (1ULL << 0),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. |
There was a problem hiding this comment.
Approach ACK fe99f68
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) |
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.
That seems sensible. I don't think we'll need to replace the |
Just so I understand, is the suggestion (for this PR) to add a typedef rather than using |
fe99f68 to
adcb4a8
Compare
|
Force-pushed to remove the test; I agree it doesn't add enough value for the additional complexity.
I'm now leaning toward keeping this patch as simple as possible, just the straightforward change from |
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 |
That's an excellent suggestion; |
|
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. |
|
Some additional historical context: #9424 (comment) |
|
I closed the current PR because #26697 was intended to replace it, but the current PR may be preferable after all, so I'm now reopening it. |
adcb4a8 to
90c3d90
Compare
|
Force pushed to make the |
Looks like the concerns expressed in #26697, which were reason for reopening here, are no-longer valid? Going to reclose this. |
8c47d59 doc: improve -debuglogfile help to be a bit clearer (jonatack) 20d89d6 bench: document expected results in logging benchmarks (jonatack) d8deba8 bench: add LogPrintfCategory and LogPrintLevel benchmarks (Jon Atack) 102b203 bench: order the logging benchmark code by output (Jon Atack) 4b3fdbf bench: update logging benchmark naming for clarity (Jon Atack) 4684aa8 bench: allow logging benchmarks to be order-independent (Larry Ruane) Pull request description: Update our logging benchmarks for evaluating ongoing work like #25203 and refactoring proposals like #26619 and #26697. - make the logging benchmarks order-independent (Larry Ruane) - add missing benchmarks for the `LogPrintLevel` and `LogPrintfCategory` macros that our logging is migrating to; at some later point it should be feasible to drop some of the previous logging benchmarks - update the logging benchmark naming to be clear which benchmark corresponds to which log macro, and update the ordering to be the same as the output - add clarifying documentation to the logging benchmarks - improve the `-debuglogfile` config option help to be clearer; can be tested by running `./src/bitcoind -help | grep -A4 '\-debuglogfile'` Reviewers can run the logging benchmarks with: ```bash ./src/bench/bench_bitcoin -filter='LogP*.*' ``` ACKs for top commit: LarryRuane: ACK 8c47d59 martinus: code review & tested ACK 8c47d59, here are my benchmark results: achow101: ACK 8c47d59 Tree-SHA512: 705f8720c9ceaf14a1945039c7578a0c17a12215cbc44908099af4ac444561c3f95d833c5a91b325cdd4470737d8a01e2da64db2d542dd7c9a3747fbfdbf213e
8c47d59 doc: improve -debuglogfile help to be a bit clearer (jonatack) 20d89d6 bench: document expected results in logging benchmarks (jonatack) d8deba8 bench: add LogPrintfCategory and LogPrintLevel benchmarks (Jon Atack) 102b203 bench: order the logging benchmark code by output (Jon Atack) 4b3fdbf bench: update logging benchmark naming for clarity (Jon Atack) 4684aa8 bench: allow logging benchmarks to be order-independent (Larry Ruane) Pull request description: Update our logging benchmarks for evaluating ongoing work like bitcoin#25203 and refactoring proposals like bitcoin#26619 and bitcoin#26697. - make the logging benchmarks order-independent (Larry Ruane) - add missing benchmarks for the `LogPrintLevel` and `LogPrintfCategory` macros that our logging is migrating to; at some later point it should be feasible to drop some of the previous logging benchmarks - update the logging benchmark naming to be clear which benchmark corresponds to which log macro, and update the ordering to be the same as the output - add clarifying documentation to the logging benchmarks - improve the `-debuglogfile` config option help to be clearer; can be tested by running `./src/bitcoind -help | grep -A4 '\-debuglogfile'` Reviewers can run the logging benchmarks with: ```bash ./src/bench/bench_bitcoin -filter='LogP*.*' ``` ACKs for top commit: LarryRuane: ACK 8c47d59 martinus: code review & tested ACK 8c47d59, here are my benchmark results: achow101: ACK 8c47d59 Tree-SHA512: 705f8720c9ceaf14a1945039c7578a0c17a12215cbc44908099af4ac444561c3f95d833c5a91b325cdd4470737d8a01e2da64db2d542dd7c9a3747fbfdbf213e
90c3d90 to
9bed303
Compare
src/logging.h
Outdated
| @@ -204,7 +204,7 @@ namespace BCLog { | |||
| void SetLogLevel(Level level) { m_log_level = level; } | |||
| bool SetLogLevel(std::string_view level); | |||
|
|
|||
| uint32_t GetCategoryMask() const { return m_categories.load(); } | |||
| uint64_t GetCategoryMask() const { return m_categories.load(); } | |||
There was a problem hiding this comment.
In commit "log: expand BCLog::LogFlags (categories) to 64 bits" (9bed303)
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.
9bed303 to
f64b861
Compare
I took this suggestion, thanks. As a test, I defined |
There was a problem hiding this comment.
ACK f64b861
I was fine with (1ULL << 23) as well. Another way is to use the 0b prefix (then no need for the ULL suffix):
enum LogFlags : CategoryMask {
NONE = 0b00000000000000000000000000000000,
NET = 0b00000000000000000000000000000001,
TOR = 0b00000000000000000000000000000010,
MEMPOOL = 0b00000000000000000000000000000100,
HTTP = 0b00000000000000000000000000001000,
....
};|
Code review ACK f64b861, just added uint64_t typedef since last review.
This seems ok, though I think would just write I also still think #26697 is a nicer alternative to this, removing hardcoded numbers and bitwise operations outside of the bitset class. |
|
Maybe |
This will increase the maximum number of logging categories from 32 to 64.
f64b861 to
b31a0cd
Compare
I like that suggestion, I just force-pushed it, thanks! |
|
ACK b31a0cd |
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.