-
Notifications
You must be signed in to change notification settings - Fork 9
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] add ffi for crossbeam::ArrayQueue #538
Conversation
BenchmarksComparisonBenchmark execution time: 2024-08-01 18:55:43 Comparing candidate commit 2b03b84 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 41 metrics, 2 unstable metrics. scenario:tags/replace_trace_tags
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
BaselineOmitted due to size. |
Library Vulnerabilities✅ No library vulnerabilities found (scanned 5d0d2e3). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #538 +/- ##
==========================================
- Coverage 70.22% 70.22% -0.01%
==========================================
Files 213 214 +1
Lines 28549 28813 +264
==========================================
+ Hits 20048 20233 +185
- Misses 8501 8580 +79
|
0d94e72
to
aaabdaf
Compare
3ecab4d
to
e812aff
Compare
89b98c9
to
aff0db3
Compare
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 looks quite reasonable! I've been holding off on buffering things on the Ruby library (instead try to make everything immediate) in part because of the complexity of getting this kind of queue going in a C codebase, but I guess after this is in I should probably consider using it in a few places :)
My rust-foo isn't great so I recommend waiting for @danielsn's final pass/approval as well, but here sir, take my 👍
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.
As a general comment, all of the extern "C"
functions in this PR take an *ArrayQueue
and cast it to an &'a crossbeam::ArrayQueue
, but the thing is, the lifetime is never bound, so you could very well get an &'static crossbeam::ArrayQueue
and have it escape the fucntion.
I think it'd be better to take an &'a ArrayQueue
in the ffi fucntions, which is translated to the same ArrayQueue*
in the C header, which would allow to derive the &'a crossbeam::ArrayQueue
lifetime safely and ensure it doesn't escape.
Can you add description and what motivates this PR please ? (as describe in the template) |
95fc303
to
bb985a6
Compare
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.
Modulo a few ⛏️ LGTM
Co-authored-by: Daniel Schwartz-Narbonne <danielsn@users.noreply.github.com>
What does this PR do?
Adds ffi for
crossbeam::ArrayQueue<*mut c_void>
Motivation
Python profiler allocates a new
Sample
whenever a it collects an event (stack/memory/lock). Then, the Sample is freed right after. Code fornew/delete Sample
is here. This results in callingnew/delete
a lot and creates some memory pressure to the system and shows up on native profiler. Frequent calls tonew/delete
can be avoided if we use a pool ofSample
s. And thisArrayQueue
will be used to implement the pool.Another option we considered is using mutex guarded vector in C++ in dd-trace-py. However, it would be little tricky to handle
fork()
if we use a lock. So we decided to use lock-free list/queue/stack.Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.