-
Notifications
You must be signed in to change notification settings - Fork 15
feat(profiling-ffi): ProfilesDictionary #1405
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:53:07 Comparing candidate commit 6289fee in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 57 metrics, 2 unstable metrics. 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 @@
## main #1405 +/- ##
==========================================
+ Coverage 71.62% 71.73% +0.10%
==========================================
Files 408 411 +3
Lines 65717 66018 +301
==========================================
+ Hits 47073 47360 +287
- Misses 18644 18658 +14
🚀 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
|
|
Seems like stacking is weird; the diffs might be messed up because the PR below this just went in, but LGTM |
- ArcHandle<ProfilesDictionary> -> ddog_prof_ProfilesDictionaryHandle - ProfileStatus -> ddog_prof_Status
7e79afb to
6289fee
Compare
|
I just rebased to fix the stacking. Sometimes it works out of the box, sometimes it doesn't :( |
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
| /// - `function` must be non-null and point to a valid `Function` for the duration of the call. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn ddog_prof_ProfilesDictionary_insert_function( | ||
| function_id: *mut FunctionId2, |
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.
Is the convention that out params go first in the list?
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.
It wasn't specifically discussed. It's pretty usual for out parameters to go early but the "this" pointer might go before it in some other languages, I'm not sure.
What does this PR do?
Adds FFI bindings for
ProfilesDictionaryand supporting infrastructure:ArcHandle<T>: Generic FFI-safe wrapper forArc<T>with transparent representation.ArcHandle<ProfilesDictionary>is renamed toddog_prof_ProfilesDictionaryHandle.ProfileStatustoddog_prof_Status.Utf8Option: UTF-8 validation strategy enum (Assume, Validate, ConvertLossy).ProfilesDictionaryFFI:ddog_prof_ProfilesDictionary_new/_drop/_try_cloneddog_prof_ProfilesDictionary_insert_str/_insert_function/_insert_mappingddog_prof_ProfilesDictionary_get_strensure_non_null_out_parameter!andensure_non_null_insert!for consistent null-pointer error handling.Motivation
This is part of the larger effort to add dictionary-based profiling APIs that enable efficient ID-based references for repeated data (strings, functions, mappings). What comes next is the
ddog_prof_Profile_add2API which uses the IDs created by this PR. This eliminates redundant string copies and enables concurrent insertion from multiple threads.Additional Notes
Depends on these PRs, merge them first:
How to test the change?
Run the comprehensive test suite: