Skip to content

Conversation

kristapsk
Copy link
Contributor

@kristapsk kristapsk commented Jan 16, 2024

Don't see any benefits here, only harder to read for most of the users.

Before:

2024-01-16T13:11:36Z Dumped mempool: 8.165e-06s to copy, 0.00224268s to dump

After:

2024-01-16T13:11:36Z Dumped mempool: 0.000s to copy, 0.002s to dump

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 16, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK glozow
Concept ACK epiccurious

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@kristapsk kristapsk changed the title Don't use scientific notation in "Dumped mempool" log message log: Don't use scientific notation in "Dumped mempool" message Jan 16, 2024
@maflcko
Copy link
Member

maflcko commented Jan 16, 2024

I don't think nanosecond precision helps here. Might as well limit to %.2fs?

Same here?

src/policy/fees.cpp:    LogPrint(BCLog::ESTIMATEFEE, "Recorded %u unconfirmed txs from mempool in %gs\n", num_entries, Ticks<SecondsDouble>(endclear - startclear));

@kristapsk
Copy link
Contributor Author

I don't think nanosecond precision helps here. Might as well limit to %.2fs?

Agree, such precision is not needed, but didn't seem too important, message itself is relatively small anyway, so few extra characters doesn't hurt.

I would prefer millisecond precision, but plain %.3f will output 0.000 for 0.0009, don't think that's the right thing to do, should ceil or round before output then.

Same here?

src/policy/fees.cpp:    LogPrint(BCLog::ESTIMATEFEE, "Recorded %u unconfirmed txs from mempool in %gs\n", num_entries, Ticks<SecondsDouble>(endclear - startclear));

Will look at it.

@glozow
Copy link
Member

glozow commented Jan 18, 2024

plain %.3f will output 0.000 for 0.0009, don't think that's the right thing to do, should ceil or round before output then.

Why? "It took less than 1ms" should be pretty intuitive.

@epiccurious
Copy link
Contributor

Tested ACK.

Before:
2024-01-31T16:34:07Z Dumped mempool: 2.007e-06s to copy, 0.00121729s to dump

With %f:
2024-01-31T16:36:03Z Dumped mempool: 0.000002s to copy, 0.001363s to dump

With %0.3f:
2024-01-31T16:38:05Z Dumped mempool: 0.000s to copy, 0.001s to dump

I would prefer millisecond precision, but plain %.3f will output 0.000 for 0.0009, don't think that's the right thing to do, should ceil or round before output then.

A ceiling function wouldn't be right either, since it would report 1ms for my test when it actually took 2 µs.

Rounding to the nearest 1ms would be nice (1.99 ms would be logged as 2ms not 1ms), but might not be worth the added code complexity for little benefit.

@maflcko
Copy link
Member

maflcko commented Jan 31, 2024

Are you still working on this, or should it be marked "up for grabs"?

@epiccurious
Copy link
Contributor

@kristapsk if you prefer, I'd be happy to see it through.

@kristapsk kristapsk changed the title log: Don't use scientific notation in "Dumped mempool" message log: Don't use scientific notation in log messages Jan 31, 2024
@kristapsk
Copy link
Contributor Author

Changed also other message and added rouding to ms precision.

Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

lgtm ACK c819a83. can you update the PR description?

@kristapsk
Copy link
Contributor Author

lgtm ACK c819a83. can you update the PR description?

Updated.

@maflcko
Copy link
Member

maflcko commented Feb 2, 2024

lgtm

@glozow
Copy link
Member

glozow commented Feb 2, 2024

lgtm ACK c819a83. can you update the PR description?

Updated.

Are you sure the 'before" is accurate?

@kristapsk
Copy link
Contributor Author

lgtm ACK c819a83. can you update the PR description?

Updated.

Are you sure the 'before" is accurate?

Yes, %g results in using %e or %f, depending on what results in shorter string.

@kristapsk
Copy link
Contributor Author

lgtm ACK c819a83. can you update the PR description?

Updated.

Are you sure the 'before" is accurate?

Yes, %g results in using %e or %f, depending on what results in shorter string.

Oh, wait, no, accidentally removed precision for "before" when editing, double checked debug.log, fixed.

@glozow glozow merged commit 9eeee7c into bitcoin:master Feb 5, 2024
@kristapsk kristapsk deleted the dumped-mempool-f branch April 24, 2024 20:59
@bitcoin bitcoin locked and limited conversation to collaborators Apr 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants