Skip to content

fuzz: improve log performance#4523

Merged
TheBlueMatt merged 2 commits intolightningdevkit:mainfrom
joostjager:fuzz-log-performance
Mar 31, 2026
Merged

fuzz: improve log performance#4523
TheBlueMatt merged 2 commits intolightningdevkit:mainfrom
joostjager:fuzz-log-performance

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented Mar 30, 2026 •
edited
Loading

It turned out that null logging during fuzzing was still a break on performance because of unconditional format!'ing. 2.5x speed up in CI.

@joostjager
Remove dead string search in fuzz SearchingOutput
b9181c3 
The searched-for log message ("Outbound update_fee HTLC buffer
overflow") no longer exists in the lightning crate, so the
from_utf8 + contains check on every log line was pure waste.

AI tools were used in preparing this commit.
@joostjager
Skip log formatting entirely for DevNull output
d6ff54e 
Even though DevNull discards the bytes, the formatting work
(SubstringFormatter, fmt::write, from_utf8) was still being done
on every log call. Short-circuit in TestLogger::log via a TypeId
check, which monomorphization resolves at compile time.

AI tools were used in preparing this commit.
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Mar 30, 2026 •
edited
Loading

👋 I see @valentinewallace was un-assigned.
If you'd like another reviewer assignment, please click here.

@joostjager joostjager mentioned this pull request Mar 30, 2026
Draft
@joostjager joostjager marked this pull request as ready for review March 30, 2026 14:29
@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

ldk-claude-review-bot commented Mar 30, 2026

No issues found.

Details:

  • fuzz/src/utils/test_logger.rs (TypeId optimization): The TypeId::of::<Out>() == TypeId::of::<DevNull>() check is correct. Since TestLogger<DevNull> is monomorphized at compile time, LLVM will constant-fold this comparison to true and eliminate the function body, making log() a no-op. Even when called through dynamic dispatch (Arc<dyn Logger>), the monomorphized implementation retains the optimization. The Output trait already has the required 'static bound for TypeId.

  • fuzz/src/chanmon_consistency.rs (SearchingOutput removal): The searched string "Outbound update_fee HTLC buffer overflow - counterparty should force-close this channel" no longer exists anywhere in the codebase, confirmed by grep. This means may_fail was always false, so all removed conditional branches were already unconditional panics. The removal is a correct dead-code cleanup with no behavior change.

  • Parameter rename (underlying_out -> out): Correct — SearchingOutput::new(underlying_out) previously produced out; now the parameter is used directly.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.21%. Comparing base (688544d) to head (d6ff54e).
⚠️ Report is 27 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4523      +/-   ##
==========================================
+ Coverage   86.14%   86.21%   +0.06%     
==========================================
  Files         160      160              
  Lines      108046   108410     +364     
  Branches   108046   108410     +364     
==========================================
+ Hits        93080    93461     +381     
+ Misses      12346    12318      -28     
- Partials     2620     2631      +11
Flag Coverage Δ
tests 86.21% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

TheBlueMatt
TheBlueMatt reviewed Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Hmm, we do want the ability to catch debug asserts in Display/Debug impls. Where is the perf improvement - is it focused on individual fuzzers?

@TheBlueMatt TheBlueMatt removed the request for review from valentinewallace March 30, 2026 14:50
@joostjager
Copy link
Copy Markdown
Contributor Author

joostjager commented Mar 30, 2026 •
edited
Loading

Hmm, we do want the ability to catch debug asserts in Display/Debug impls. Where is the perf improvement - is it focused on individual fuzzers?

For the fuzz CI job for chanmon consistency, it goes down from 10 to 4 minutes. There is just a lot being formatted, but then not actually logged. For catching asserts in display/debug, fuzzing seems not the most efficient way to do it.

TheBlueMatt
TheBlueMatt approved these changes Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Hmm, yea, that seems like the wrong tradeoff.

@TheBlueMatt TheBlueMatt merged commit 4501dc7 into lightningdevkit:main Mar 31, 2026
21 of 23 checks passed
@joostjager joostjager self-assigned this Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

Assignees

@joostjager joostjager

Labels

None yet

Projects

Weekly Goals
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@joostjager @ldk-reviews-bot @ldk-claude-review-bot @TheBlueMatt