-
Notifications
You must be signed in to change notification settings - Fork 160
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
feat(profiling) move upscaling to libdatadog #1984
Conversation
This will remove upscaling for `alloc-size` and `alloc-samples` profile types from the library and make use of the upscaling capabilities in `libdatadog`. Doing this solves the rounding issues with the current implementation due to float -> integer conversion, therefore allocation profiles will be more correct and it offloads upscaling from the main thread to the serialization step.
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.
Do we have an understanding of how this changes performance?
Did a benchmark on CLI using a script that does a lot of allocations (therefor a lot of samples in allocation profiling) $ hyperfine --runs 20 --warmup 2 "php -d extension=datadog-profiling-old.dylib test.php" "php -d extension=datadog-profiling.dylib test.php"
Benchmark 1: php -d extension=datadog-profiling-old.dylib test.php
Time (mean ± σ): 420.1 ms ± 5.1 ms [User: 461.9 ms, System: 29.1 ms]
Range (min … max): 416.6 ms … 436.3 ms 20 runs
Benchmark 2: php -d extension=datadog-profiling.dylib test.php
Time (mean ± σ): 416.2 ms ± 2.4 ms [User: 460.5 ms, System: 26.3 ms]
Range (min … max): 414.7 ms … 425.5 ms 20 runs
Summary
'php -d extension=datadog-profiling.dylib test.php' ran
1.01 ± 0.01 times faster than 'php -d extension=datadog-profiling-old.dylib test.php' The |
Depending on the machine the test runs on, it could be possible that we either take a sample or not. We don't care for this test, but we actually tested that we do not take a sample, which might fail this test in case we do
The old version with upscaling in the library: Transactions: 58 hits
Availability: 100.00 %
Elapsed time: 10.16 secs
Data transferred: 1.50 MB
Response time: 0.17 secs
Transaction rate: 5.71 trans/sec
Throughput: 0.15 MB/sec
Concurrency: 0.99
Successful transactions: 58
Failed transactions: 0
Longest transaction: 0.31
Shortest transaction: 0.14 The new version with upscaling in Transactions: 63 hits
Availability: 100.00 %
Elapsed time: 10.87 secs
Data transferred: 1.63 MB
Response time: 0.17 secs
Transaction rate: 5.80 trans/sec
Throughput: 0.15 MB/sec
Concurrency: 0.99
Successful transactions: 63
Failed transactions: 0
Longest transaction: 0.30
Shortest transaction: 0.14 The difference in the Note in the benchmarks above that the "new" version was running |
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 long as you think the upscaled numbers we are seeing make sense, then I'm good to merge this!
Description
This will remove upscaling for
alloc-size
andalloc-samples
profile types from the library and make use of the upscaling capabilities inlibdatadog
.Doing this solves the rounding issues with the current implementation due to float -> integer conversion, therefore allocation profiles will be more correct and it offloads upscaling from the main thread to the serialization step.
Also bumps
libdatadog
to2.1.0
Readiness checklist
Reviewer checklist
Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.