[PROF-8328] Import java-profiler PID controller and port it to C #3190
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…filer These were taken from: * https://github.com/DataDog/java-profiler/blob/11fe6206c31a14c6e5134e8401eaec8b22c618d7/ddprof-lib/src/main/cpp/pidController.h * https://github.com/DataDog/java-profiler/blob/11fe6206c31a14c6e5134e8401eaec8b22c618d7/ddprof-lib/src/main/cpp/pidController.cpp ...and do not yet compile (since they're C++).
As much as possible, I've tried to retain the original code shape and conventions, even though it doesn't quite match the rest of the Ruby profiler codebase.
Codecov Report
@@ Coverage Diff @@
## master #3190 +/- ##
==========================================
- Coverage 98.21% 98.19% -0.02%
==========================================
Files 1247 1250 +3
Lines 71664 71910 +246
Branches 3329 3355 +26
==========================================
+ Hits 70382 70610 +228
- Misses 1282 1300 +18
... and 21 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ivoanjo
added a commit
that referenced
this pull request
Feb 9, 2024
**What does this PR do?** This PR removes the unused profiler PID controller implementation. **Motivation:** In #3190 we imported the PID controller, and planned to use it to control the allocation profiler. In the end, we ended up going with a different solution for that (#3395) so let's remove the PID controller for now. We can always revert this commit if we need it again. **Additional Notes:** N/A **How to test the change?** Validate that CI is still green :)
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR:
pidController.handpidController.cppfrom the java-profiler libraryPidControllerclass to C, to match the rest of the Ruby profilerI've recorded the porting steps in multiple commits, and as much as possible tried to keep the conventions and structure of the original code.
In practice, this does not yet change anything in the Ruby profiler, since these files are not yet used.
Motivation:
I plan to use the PID controller to control how often we take samples in the allocation profiler, like the Java profiler does.
Additional Notes:
I plan to ask the folks from the Java profiler to review this one :)
How to test the change?
This code is not yet used; it'll get tested when it gets integrated into the allocation profiler.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.