Skip to content

Conversation

@not-matthias
Copy link
Member

No description provided.

@not-matthias not-matthias changed the base branch from master to cod-578-fix-span-ordering-on-the-flamegraph October 31, 2025 11:32
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 31, 2025

CodSpeed Performance Report

Merging #4 will improve performances by ×6.9

Comparing cod-1573-v431-seems-to-cause-hangs (77811f8) with add-benchmarks (f16dd47)

Summary

⚡ 12 improvements
✅ 56 untouched
🆕 4 new

Benchmarks breakdown

Benchmark BASE HEAD Change
test_valgrind[valgrind-3.25.1, python3 testdata/test.py, full-no-inline] 5.8 s 5.2 s +10.94%
test_valgrind[valgrind-3.25.1.codspeed, echo Hello, World!, full-with-inline] 852.6 ms 726.4 ms +17.38%
test_valgrind[valgrind-3.25.1.codspeed, echo Hello, World!, inline] 786.8 ms 660.6 ms +19.11%
test_valgrind[valgrind-3.25.1.codspeed, ls bench.py, full-with-inline] 931.9 ms 787.9 ms +18.29%
test_valgrind[valgrind-3.25.1.codspeed, ls bench.py, inline] 849.7 ms 707.3 ms +20.14%
test_valgrind[valgrind-3.25.1.codspeed, stress-ng --cpu 1 --timeout 1s, full-no-inline] 7.1 s 3.1 s ×2.3
test_valgrind[valgrind-3.25.1.codspeed, stress-ng --cpu 1 --timeout 1s, full-with-inline] 3.8 s 3.4 s +10.15%
test_valgrind[valgrind-3.25.1.codspeed, stress-ng --cpu 1 --timeout 1s, inline] 2.7 s 2.2 s +20.26%
test_valgrind[valgrind-3.25.1.codspeed, stress-ng --cpu 4 --timeout 1s, full-with-inline] 3.8 s 3.4 s +14.15%
test_valgrind[valgrind-3.25.1.codspeed, stress-ng --cpu 4 --timeout 1s, inline] 2.7 s 2.3 s +19.44%
test_valgrind[valgrind-3.25.1.codspeed, testdata/take_strings-aarch64 varbinview_non_null, full-with-inline] 49.5 s 7.7 s ×6.4
test_valgrind[valgrind-3.25.1.codspeed, testdata/take_strings-aarch64 varbinview_non_null, inline] 49.4 s 7.1 s ×6.9
🆕 test_valgrind[valgrind-3.26.0, python3 testdata/test.py, full-no-inline] N/A 5.3 s N/A
🆕 test_valgrind[valgrind-3.26.0, python3 testdata/test.py, full-with-inline] N/A 5.5 s N/A
🆕 test_valgrind[valgrind-3.26.0, python3 testdata/test.py, inline] N/A 3.9 s N/A
🆕 test_valgrind[valgrind-3.26.0, python3 testdata/test.py, no-inline] N/A 3.6 s N/A

@not-matthias not-matthias requested a review from Copilot October 31, 2025 13:33
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes inline function lookups in Valgrind's debug info system by replacing O(n) linear search with a hash table cache backed by binary search fallback. The implementation uses the Verstable hash table library and adds benchmarking infrastructure using CodSpeed.

  • Added hash table-based caching for inline function address lookups
  • Integrated Verstable (v2.2.1) hash table library for efficient O(1) lookups
  • Implemented hybrid lookup strategy: hash table cache with binary search fallback
  • Added CodSpeed benchmarking workflow and test infrastructure

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
coregrind/m_debuginfo/verstable.h Added Verstable v2.2.1 hash table library for efficient key-value storage
coregrind/m_debuginfo/storage.c Build hash table during inltab canonicalization to cache addr_lo→index mappings
coregrind/m_debuginfo/priv_storage.h Added inltab_lookup field to DebugInfo struct for hash table storage
coregrind/m_debuginfo/priv_inltab_lookup.h Public API for inline table lookup with hash table and binary search
coregrind/m_debuginfo/inltab_lookup.c Implementation of hybrid lookup using hash table cache and binary search fallback
coregrind/m_debuginfo/debuginfo.c Updated VG_(get_inline_fnname) to use new hash table lookup API
bench/testdata/take_strings Git LFS binary test data for benchmarks
bench/bench.py Python benchmark script using pytest-codspeed
.github/workflows/codspeed.yml CodSpeed benchmark CI workflow
.github/workflows/ci.yml Updated CI to skip installing package docs
.gitattributes Git LFS configuration for test data

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch 3 times, most recently from 670e716 to e2da712 Compare October 31, 2025 15:30
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from cd11b8b to e79f8d9 Compare October 31, 2025 15:51
Copy link

@GuillaumeLagrange GuillaumeLagrange left a comment

Choose a reason for hiding this comment

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

olgtm, will approve once we have at lesat tested teh impact compared to baseline valgrind.

Comments are pretty minor

@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from 5c15079 to c65ef14 Compare November 3, 2025 10:03
@not-matthias not-matthias changed the base branch from cod-578-fix-span-ordering-on-the-flamegraph to master November 3, 2025 10:04
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from c65ef14 to 6a89687 Compare November 3, 2025 10:05
Copy link
Member

@art049 art049 left a comment

Choose a reason for hiding this comment

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

Code changes lgtm! Got some feedback on the devX though

@art049
Copy link
Member

art049 commented Nov 3, 2025

We also need a few addtional binaries to bench IMO

@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from fcc25c2 to 5d52ed7 Compare November 3, 2025 13:44
@not-matthias
Copy link
Member Author

not-matthias commented Nov 3, 2025

We also need a few addtional binaries to bench IMO

I've added ls and echo to the test binaries, lmk if you want anyting else as well. I've also modified the bench script a bit to allow passing an arbitrary cmd which will be useful for cmds with args.

EDIT: Also added caching and disabled the take-strings benchmark for 3.25 since it takes a super long time.

@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch 5 times, most recently from 0157df9 to 742df7b Compare November 3, 2025 19:19
Copy link

@GuillaumeLagrange GuillaumeLagrange left a comment

Choose a reason for hiding this comment

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

lgtm, very good work 🤗

@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch 2 times, most recently from e5a5e55 to c104dbd Compare November 4, 2025 10:43
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch 7 times, most recently from 76d83b9 to 6f97311 Compare November 4, 2025 15:34
@not-matthias not-matthias changed the base branch from master to add-benchmarks November 4, 2025 15:34
@not-matthias not-matthias force-pushed the add-benchmarks branch 5 times, most recently from 09fe459 to 093f525 Compare November 4, 2025 18:22
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from 6f97311 to d0504d7 Compare November 4, 2025 18:42
@not-matthias not-matthias force-pushed the add-benchmarks branch 2 times, most recently from fce6155 to e16a7f8 Compare November 5, 2025 10:59
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from d0504d7 to e96b4c1 Compare November 5, 2025 11:18
@not-matthias not-matthias force-pushed the cod-1573-v431-seems-to-cause-hangs branch from e96b4c1 to 77811f8 Compare November 5, 2025 13:26
Base automatically changed from add-benchmarks to master November 5, 2025 14:41
@not-matthias not-matthias merged commit 77811f8 into master Nov 5, 2025
35 of 37 checks passed
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