perf(profiling)!: use Function2 and Mapping2 from ProfilesDictionary#1594
Draft
morrisonlevi wants to merge 9 commits intolevi/api2-eq-hashfrom
Draft
perf(profiling)!: use Function2 and Mapping2 from ProfilesDictionary#1594morrisonlevi wants to merge 9 commits intolevi/api2-eq-hashfrom
morrisonlevi wants to merge 9 commits intolevi/api2-eq-hashfrom
Conversation
Ensure Profile::try_new_internal always creates and stores a ProfilesDictionaryTranslator when one is not provided. This guarantees dictionary-backed api2 paths are available from profile construction time and removes constructor-time optional behavior. Add a focused unit test that verifies Profile::new initializes the profiles dictionary.
Convert legacy api::Sample inputs into api2 structures via the profile dictionary, then route try_add_sample through try_add_sample2. This unifies ingestion behavior across APIs while keeping dictionary-backed ids on a single translation path. Remove the now-unused legacy try_add_mapping/function/location helpers from the old ingestion path.
Extend StringTable with an optional ProfilesDictionary attachment and add try_intern_string_id2/try_intern_string_ref for dictionary-backed ids. Return a dedicated ProfilesDictionaryRequired error when no dictionary is attached. Attach each Profile dictionary to its string table at construction so dictionary-backed interning can reuse dictionary bytes safely. Add focused tests for required-dictionary errors and dictionary interning behavior.
…ed internals Consolidate profiling and profiling-ffi around dictionary-backed ids, remove legacy managed-string and generational interning code paths, and update tests, fuzzing, and benchmarks for the new profile add-sample flow. Co-authored-by: Cursor <cursoragent@cursor.com>
Make ProfilesDictionary mandatory in StringTable construction and remove optional attach/error paths so dictionary-backed interning follows one invariant. Co-authored-by: Cursor <cursoragent@cursor.com>
Revert small doc and derive differences in FunctionId2, MappingId2, and StringId2 so this branch only carries the intended profiling refactor work. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore explicit common.h include in profile_intern.cpp, simplify a redundant api import style, and improve StringTable test expect messages. Also drop the cxx profiling comment tweak to keep this PR focused. Fix clippy warnings. Co-authored-by: Cursor <cursoragent@cursor.com>
Implement Send at the Profile container level rather than on raw-pointer id handles, and document why this is safe with dictionary-owned stable storage. Add a compile-time test asserting Profile implements Send. Co-authored-by: Cursor <cursoragent@cursor.com>
Update the interning_strings benchmark to construct a ProfilesDictionary Arc and pass it into StringTable::new so benches compile with the required dictionary-backed constructor. Co-authored-by: Cursor <cursoragent@cursor.com>
📚 Documentation Check Results📦
|
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
🔒 Cargo Deny Results📦
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## levi/api2-eq-hash #1594 +/- ##
=====================================================
+ Coverage 70.90% 71.36% +0.45%
=====================================================
Files 424 418 -6
Lines 62053 61375 -678
=====================================================
- Hits 44001 43802 -199
+ Misses 18052 17573 -479
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
+635 -2,246, which gives you an idea of how much is relatively removed.Motivation
Performance and simpler data flow.
On
main, we translate functions and mappings from the long-lived dictionary into the short-livedProfileinternals. This translation mainly exists for backward compatibility. By dropping that compatibility path, we can remove the extra conversion work and improve performance.Here's the benchmark command:
And the results:
main[302.93 µs 306.01 µs 310.63 µs]levi/profiling-remove-legacy-ids[237.07 µs 241.90 µs 247.83 µs]-22.426% to -20.044%In other words, adding samples using the
try_add_sample2API is now faster, with the median latency going from306.01 µsto241.90 µs(~21% faster on this branch).Note that the older
try_add_samplepath is slower, which is something to evaluate. I hypothesized that it wouldn't change much: it was interning before, and it's interning still, just in a ProfilesDictionary instead. However, the thread-safety of the ProfilesDictionary slows it down more than I expected.Additional Notes
I intend to have AI help me migrate the Ruby profiler to test these changes.
How to test the change?
For now, don't. This is a work-in-progress.