-
Notifications
You must be signed in to change notification settings - Fork 15
feat(profiling-ffi): Profile_add2 & Profile_with_dictionary #1406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
BenchmarksComparisonBenchmark execution time: 2025-12-17 19:56:15 Comparing candidate commit 0618837 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 56 metrics, 2 unstable metrics. scenario:benching deserializing traces from msgpack to their internal representation
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
Group 14
Group 15
Group 16
Group 17
Group 18
Group 19
BaselineOmitted due to size. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## levi/profiles-dictionary-ffi #1406 +/- ##
================================================================
- Coverage 71.73% 71.67% -0.07%
================================================================
Files 411 411
Lines 66018 66069 +51
================================================================
- Hits 47360 47354 -6
- Misses 18658 18715 +57
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-apple-darwin
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-apple-darwin
x86_64-unknown-linux-gnu
|
- ArcHandle<ProfilesDictionary> -> ddog_prof_ProfilesDictionaryHandle - ProfileStatus -> ddog_prof_Status
7e79afb to
6289fee
Compare
e7fc7a4 to
0618837
Compare
| // Create a ProfilesDictionary for the new API | ||
| ddog_prof_ProfilesDictionaryHandle dict = {0}; | ||
| ddog_prof_Status dict_status = ddog_prof_ProfilesDictionary_new(&dict); | ||
| if (dict_status.flags != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be a named constant to be clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or a helper function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You specifically mean the 0 for the name? It was chosen specifically because environments like Java and sometimes .NET don't get to use the C macros--the detail needs to be concrete.
But we can still add a constant, it just needs to be guaranteed to also be zero or else there could be issues.
To be honest, we could probably simplify this stuff. Rely on null to mean no error, and then use the lowest bit for "allocated or not".
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
levi.morrison@datadoghq.com cancelled this merge request build |
|
/merge --cancel |
|
View all feedbacks in Devflow UI.
|
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What does this PR do?
Adds FFI bindings for dictionary-based profiling APIs, which enable more efficient profile creation by pre-registering strings, functions, and mappings in a shared dictionary.
ddog_prof_Profile_with_dictionary: create a profile with aProfilesDictionaryfor string/function/mapping reuse.ddog_prof_Profile_add2: Add samples using dictionary IDs instead of raw strings/pointers.Label2andSample2for ID-based sample construction.Motivation
This completes the FFI layer for the dictionary-based profiling API introduced in earlier PRs. The new APIs allow profiling clients to:
ProfilesDictionaryfor deduplication. This can and should live beyond a minute, possibly forever.StringId2,FunctionId2,MappingId2) instead of raw strings.The C example demonstrates both the old and new APIs side-by-side, showing how to use the dictionary for sample insertion.
Additional Notes
libdd-profiling/benches/add_samples.rs(already inmain) show ~31% overhead for dictionary translation, primarily due to string lookups.profiles.cexample successfully compiles and runs with both APIs.How to test the change?