[Profiler] Implement interning API #917
Open
Conversation
This file contains 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
BenchmarksComparisonBenchmark execution time: 2025-03-14 20:03:14 Comparing candidate commit d4bdb5c in PR branch Found 0 performance improvements and 6 performance regressions! Performance is the same for 46 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/378282246310005
scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/x371413321323331
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #917 +/- ##
==========================================
- Coverage 72.44% 72.29% -0.16%
==========================================
Files 333 338 +5
Lines 50097 51418 +1321
==========================================
+ Hits 36294 37172 +878
- Misses 13803 14246 +443
🚀 New features to boost your workflow:
|
taegyunkim
reviewed
Mar 11, 2025
| }; | ||
| use function_name::named; | ||
|
|
||
| /// This functions interns its argument into the profiler. |
There was a problem hiding this comment.
Suggested change
| /// This functions interns its argument into the profiler. | |
| /// This function interns its argument into the profiler. |
Few more in this file.
taegyunkim
reviewed
Mar 11, 2025
There was a problem hiding this comment.
This reminded me of @ivoanjo 's ManagedStringStorage.
Correct me if I understood it wrong.
- this change uses
strings: StringTablewhere ivo's PR usesstring_storage: Option<...<...<ManagedStringStorage>>>frominternal::Profile. - we create a new
StringTablefor each Profile, whilestring_storagepersists across Profiles
ivoanjo
reviewed
Mar 13, 2025
Comment on lines
+241
to
+261
| /// This functions interns its argument into the profiler. | ||
| /// If successful, it an opaque interning ID. | ||
| /// This ID is valid for use on this profiler, until the profiler is reset. | ||
| /// It is an error to use this id after the profiler has been reset, or on a different profiler. | ||
| /// On error, it holds an error message in the error variant. | ||
| /// | ||
| /// # Safety | ||
| /// The `profile` ptr must point to a valid Profile object created by this | ||
| /// module. | ||
| /// All other arguments must remain valid for the length of this call. | ||
| /// This call is _NOT_ thread-safe. | ||
| #[must_use] | ||
| #[no_mangle] | ||
| #[named] | ||
| pub unsafe extern "C" fn ddog_prof_Profile_intern_sample( | ||
| profile: *mut Profile, | ||
| stacktrace: GenerationalId<StackTraceId>, | ||
| values: Slice<i64>, | ||
| labels: GenerationalId<LabelSetId>, | ||
| timestamp: Option<NonZeroI64>, | ||
| ) -> VoidResult { |
There was a problem hiding this comment.
"If successful, it an opaque interning ID." -> doesn't apply to this one, which returns VoidResult (also needs a bit of a text pass -- shows up in other places ;D )
ivoanjo
reviewed
Mar 13, 2025
ivoanjo
reviewed
Mar 13, 2025
Comment on lines
+33
to
+67
| pub fn intern_label_num( | ||
| &mut self, | ||
| key: GenerationalId<StringId>, | ||
| val: i64, | ||
| unit: Option<GenerationalId<StringId>>, | ||
| ) -> anyhow::Result<GenerationalId<LabelId>> { | ||
| let key = key.get(self.generation)?; | ||
| let unit = unit.map(|u| u.get(self.generation)).transpose()?; | ||
| let id = self.labels.dedup(Label::num(key, val, unit)); | ||
| Ok(GenerationalId::new(id, self.generation)) | ||
| } | ||
|
|
||
| pub fn intern_label_str( | ||
| &mut self, | ||
| key: GenerationalId<StringId>, | ||
| val: GenerationalId<StringId>, | ||
| ) -> anyhow::Result<GenerationalId<LabelId>> { | ||
| let key = key.get(self.generation)?; | ||
| let val = val.get(self.generation)?; | ||
| let id = self.labels.dedup(Label::str(key, val)); | ||
| Ok(GenerationalId::new(id, self.generation)) | ||
| } | ||
|
|
||
| pub fn intern_labelset( | ||
| &mut self, | ||
| labels: &[GenerationalId<LabelId>], | ||
| ) -> anyhow::Result<GenerationalId<LabelSetId>> { | ||
| let labels = labels | ||
| .iter() | ||
| .map(|l| l.get(self.generation)) | ||
| .collect::<anyhow::Result<Vec<_>>>()?; | ||
| let labels = LabelSet::new(labels); | ||
| let id = self.label_sets.dedup(labels); | ||
| Ok(GenerationalId::new(id, self.generation)) | ||
| } |
There was a problem hiding this comment.
Using these functions skips the Profile::validate_sample_labels that the add would check before allowing them. I've noticed this because I had a test for this failure being handled correctly on the Ruby side, and the test did not see the error it expected.
There was a problem hiding this comment.
My feeling was that these checks should maybe be a debug_assert, but running them on every insert in prod is wasted CPU cycles
danielsn
force-pushed
the
dsn/r_and_d_week_mar_2024
branch
from
8145a59 to
33380b9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
Adds a new API for adding values to the profiler which interns the value, and then returns a handle to the interned value.
Motivation
The current API has two notable downsides
This new API follows more of a builder pattern: you give a value, intern it, and then get a handle to the interned value which you can then reuse as needed.
Additional Notes
The new API is twice as fast as the old one on the C/C++ example, which do similar work
How to test the change?
The new C++ file that drives the API.