Skip to content

Conversation

@NikhilSharmaWe
Copy link
Contributor

@NikhilSharmaWe
Copy link
Contributor Author

NikhilSharmaWe commented Dec 6, 2025

@michaelsproul Could you please take a look at the changes and share your review?

@michaelsproul
Copy link
Member

Yep will do when I'm back at work on monday

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Can you also run the benchmarks before/after this change to confirm no regressions from the recalculation of depth? Thanks

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.26%. Comparing base (dbe9e15) to head (286b6d8).

Files with missing lines Patch % Lines
src/list.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
- Coverage   70.38%   70.26%   -0.12%     
==========================================
  Files          22       22              
  Lines        1263     1268       +5     
==========================================
+ Hits          889      891       +2     
- Misses        374      377       +3     

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

@NikhilSharmaWe
Copy link
Contributor Author

Ran all benchmarks, and they show no significant performance difference

Benchmark Comparison Results

Summary

Benchmarks were run before and after the change to verify no performance regression from depth recalculation in from_parts.

Conclusion: Benchmarks show no significant performance regression from the depth recalculation change. Most benchmarks are within ±5% (measurement noise), with a few showing minor variations (±3-10%) that are consistent with normal benchmark variance. The depth recalculation (Self::depth()) is a lightweight computation (log2 calculation + simple arithmetic) and has negligible impact on overall performance.

Pop Front Benchmarks

Benchmark Before After Change
pop_front_noop 8.7448 ns 8.7950 ns 8.8545 ns 8.4472 ns 8.4680 ns 8.4877 ns -3.72%
pop_front_3 50.184 ms 51.251 ms 53.013 ms 49.990 ms 50.627 ms 51.280 ms -1.22%
pop_front_4 17.285 ms 17.440 ms 17.602 ms 17.746 ms 18.128 ms 18.686 ms +3.94%
pop_front_32 1.8454 ms 1.8575 ms 1.8712 ms 1.8825 ms 1.9052 ms 1.9431 ms +2.57%
pop_front_400k 207.57 µs 208.13 µs 208.77 µs 207.98 µs 209.01 µs 210.15 µs +0.42%

Rebase Benchmarks

Benchmark Before After Change
rebase_identical 2.7484 ms 2.7951 ms 2.8531 ms 2.8513 ms 2.9670 ms 3.1067 ms +6.15%
rebase_push_1_back 2.6459 ms 2.6551 ms 2.6647 ms 2.8188 ms 2.9218 ms 3.0607 ms +10.04%
rebase_mutate0 2.6559 ms 2.6655 ms 2.6761 ms 2.8328 ms 2.8740 ms 2.9200 ms +7.82%
rebase_completely_different 2.5820 ms 2.5916 ms 2.6022 ms 2.6007 ms 2.6177 ms 2.6366 ms +1.01%

Ssz Benchmarks

Benchmark Before After Change
ssz_encode_list 5.5880 ms 5.6069 ms 5.6263 ms 5.7236 ms 5.7879 ms 5.8812 ms +3.23%
ssz_encode_decode_list 68.756 ms 70.369 ms 72.264 ms 67.380 ms 68.355 ms 69.840 ms -2.86%
ssz_encode_vector 5.6651 ms 5.6859 ms 5.7073 ms 5.6601 ms 5.6843 ms 5.7099 ms -0.03%
ssz_encode_decode_vector 67.645 ms 69.248 ms 71.791 ms 69.886 ms 72.122 ms 74.793 ms +4.15%
ssz_encode_variable_list 1.3301 ms 1.3368 ms 1.3438 ms 1.3807 ms 1.4608 ms 1.5629 ms +9.28%
ssz_encode_decode_variable_list 3.4752 ms 3.4939 ms 3.5143 ms 3.5197 ms 3.5421 ms 3.5655 ms +1.38%

Tree Hash Root Benchmarks

Benchmark Before After Change
tree_hash_root_list 61.531 ms 62.174 ms 62.844 ms 63.420 ms 64.221 ms 65.165 ms +3.29%
tree_hash_root_vector 62.085 ms 62.651 ms 63.208 ms 62.932 ms 65.209 ms 68.551 ms +4.08%
tree_hash_root_variable_list 37.600 ms 37.985 ms 38.667 ms 38.435 ms 38.570 ms 38.714 ms +1.54%
tree_hash_root_list_parallel 77.899 ms 78.388 ms 78.870 ms 79.109 ms 81.613 ms 85.224 ms +4.11%

@NikhilSharmaWe
Copy link
Contributor Author

@michaelsproul Quick ping on this PR
The requested updates have been done. Would appreciate a follow-up review when time allows.

@michaelsproul
Copy link
Member

I'm reluctant to merge something with a 10% performance penalty. In my view that is quite a lot, and these functions are on the critical path inside Lighthouse. If a PR were opened with a 10% performance benefit I would probably be inclined to merge it, even if it made the API slightly less ergonomic.

I think maybe the original issue is a bit of a non-issue as well. This is an internal API that is currently used safely. What do you think?

@NikhilSharmaWe
Copy link
Contributor Author

@michaelsproul Yes, that seems reasonable.
I can close the PR if you want.

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.

List::from_parts could drop the depth parameter

3 participants