-
Notifications
You must be signed in to change notification settings - Fork 12
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
[profiling] Reduce copying and allocation in exporter #926
Conversation
BenchmarksComparisonBenchmark execution time: 2025-03-20 21:39:17 Comparing candidate commit 26375b3 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 50 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/ 378282246310005
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. |
8945950
to
0a70e3e
Compare
0a70e3e
to
dfa6eb3
Compare
dfa6eb3
to
606d179
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #926 +/- ##
==========================================
+ Coverage 72.91% 72.98% +0.07%
==========================================
Files 334 334
Lines 51002 50915 -87
==========================================
- Hits 37186 37160 -26
+ Misses 13816 13755 -61
🚀 New features to boost your workflow:
|
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.
Gave it a pass!
Overall I like this PR less about the copying/allocation reduction and more as I think it's very useful to have a direct link between the profile and the exporter. In the past for instance we've had to expose the ProfiledEndpointsStats
because it was not encoded in the pprof. With this change, we can trivially report more things (metrics? other info?) that also don't get encoded in the pprof.
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
i686-alpine-linux-musl
i686-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
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.
Left a few notes!
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.
Nice work! I've marked it as Request changes
because of the invalid-report-without-start/end thingy, but after clearing that this is basically good to go.
Co-authored-by: Ivo Anjo <[email protected]>
Co-authored-by: Ivo Anjo <[email protected]>
…ation **What does this PR do?** In #926 we removed the `start_time` argument from * `ddog_prof_Profile_new` * `ddog_prof_Profile_with_string_storage` * `ddog_prof_Profile_reset` The intention was that having it as an argument in `ddog_prof_Profile_serialize` was enough, and anyway almost everyone was passing in `null`s in the APIs above. I missed when suggesting in that PR that the `start_time` for serialize was the `start_time` for the next profile, not the one being serialized. In this PR I'm changing that behavior: the `start_time` argument to serialize now controls the time for the profile being serialized, allowing the profiling library to have exact control over this value. I've also removed the duration. [As can be seen in this github search](https://github.com/search?q=org%3ADataDog+ddog_prof_Profile_serialize&type=code) this is not expected to impact anyone: everyone's passing `NULL` for `start_time` and `duration` when calling serialize already. **Motivation:** Allow Ruby profiler to set the start_time of profiles. **Additional Notes:** N/A **How to test the change?** I've tested this with my experimental libdatadog 17 branch for Ruby.
…ation (#963) **What does this PR do?** In #926 we removed the `start_time` argument from * `ddog_prof_Profile_new` * `ddog_prof_Profile_with_string_storage` * `ddog_prof_Profile_reset` The intention was that having it as an argument in `ddog_prof_Profile_serialize` was enough, and anyway almost everyone was passing in `null`s in the APIs above. I missed when suggesting in that PR that the `start_time` for serialize was the `start_time` for the next profile, not the one being serialized. In this PR I'm changing that behavior: the `start_time` argument to serialize now controls the time for the profile being serialized, allowing the profiling library to have exact control over this value. I've also removed the duration. [As can be seen in this github search](https://github.com/search?q=org%3ADataDog+ddog_prof_Profile_serialize&type=code) this is not expected to impact anyone: everyone's passing `NULL` for `start_time` and `duration` when calling serialize already. **Motivation:** Allow Ruby profiler to set the start_time of profiles. **Additional Notes:** N/A **How to test the change?** I've tested this with my experimental libdatadog 17 branch for Ruby.
What does this PR do?
Passes the
EncodedProfile
as a Rust object, rather than forcing it through the C-FFI straw.Motivation
We ended up having to make a new
Vec
and copy the bytes from the pprof in, even though they were already there. This saves the allocation and copy (which can be several MB).Additional Notes
Anything else we should know when reviewing?
How to test the change?
Existing tests