bench: borrow inputs in the benches that only read them - #849
Conversation
Follow-up to #837: eight benches still took their pre-built input by value, so dropping it (a full Node tree, wa::Message structs) was timed as part of the operation. The get_children_by_tag flamegraph made it visible: 66% of the reported time was drop_in_place of the input tree, and the traversal itself is ~2 us, not 57 us. Inputs that the operation only reads are now passed by reference, and produced values are returned so the harness drops them outside the window; process_history_sync keeps bench_values because the operation genuinely consumes its blob.
📝 WalkthroughSummary by CodeRabbitRelease Notes
Note: These are internal testing infrastructure improvements with no impact on end-user functionality. WalkthroughThese changes convert benchmark harnesses across two files from divan's ChangesBenchmark Harness Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merging this PR will improve performance by 54.01%
Performance Changes
Tip Curious why this is faster? Comment Comparing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/benches/reporting_token_benchmark.rs`:
- Line 54: The closure passed to .bench_refs currently calls
black_box(generate_reporting_token_content(msg)) but doesn't return the
generated value, so destruction happens inside the timed section; change these
closures (the ones invoking generate_reporting_token_content via .bench_refs at
the occurrences noted and the similar cases at the other two locations) to
return the produced token (e.g., return the Vec or token value) instead of only
black_box-ing it, so the benchmark measures creation cost while drop happens
outside the timed window—mirror the approach used in bench_jid_to_owned_access
where the produced value is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e87596b7-f23a-4c6a-8c6a-90e2bdf0055e
📒 Files selected for processing (2)
wacore/benches/reporting_token_benchmark.rswacore/binary/benches/binary_benchmark.rs
Follow-up to #837, found while flamegraphing
bench_get_children_by_tag(57.1 us in Simulation): 66% of that time isdrop_in_placeof the input Node tree, because the bench consumed its pre-built input withbench_valuesand the drop ran inside the measured window. The actual traversal costs about 2 us.Eight benches had the pattern:
get_children_by_tag,jid_to_owned_access, the six reporting-token benches (content extraction, full generation, message encoding). All of them only read the input, so they now usebench_refs, and values produced inside the closure are returned so divan drops them outside the timing.process_history_synckeepsbench_values: that operation genuinely takes ownership of its blob, so the consume is part of the measured contract (and its result was already returned).The CodSpeed report on this PR will show large "improvements" on these eight benchmarks; that is measurement correction, not a performance change. No library code is touched.