Skip to content

fuzz: improve log performance#4523

Open
joostjager wants to merge 2 commits intolightningdevkit:mainfrom
joostjager:fuzz-log-performance
Open

fuzz: improve log performance#4523
joostjager wants to merge 2 commits intolightningdevkit:mainfrom
joostjager:fuzz-log-performance

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented Mar 30, 2026

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.

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.
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

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

@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

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

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.21%. Comparing base (688544d) to head (d6ff54e).
⚠️ Report is 8 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.

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants