-
Notifications
You must be signed in to change notification settings - Fork 380
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: Include specific kind of VM-internal T_IMEMO object in allocation info #3290
Conversation
**What does this PR do?** This PR extends the code that generates a description for an object during allocation to include the specific kind of `T_IMEMO` object. On Ruby < 3, the `rb_imemo_name` function we're relying on does not exist, so I chose to just not gather this information for legacy Rubies. (It's posssible to still get it, but I didn't want to spend more time). **Motivation:** During discussion of #3282 the question of what a `T_IMEMO` is came up, and since it turns out it can represent different kinds of things in the Ruby VM, I decided to see if there was a way of spelling those out, and it turns out there is. Is this going to be useful for most customers? Probably not, but even for our internal debugging/learning, it may come in handy, so it was a bit of a "why not" kinda thing. **Additional Notes:** N/A **How to test the change?** I hesitated in terms of testing this, since these objects are internally created by the VM so we could only run a bit of code and kinda hope to trigger their creation. Since this was a kind-of small thing I... decided to just leave it as-is without a specific test. In the future, we may add some testing, if we think this feature is useful enough. I did test it locally -- on Ruby 3.x I got from this trivial script `DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e "def foo; end; foo; sleep(1)"` (output from `go pprof tool`): ``` allocation class:[(VM Internal, T_IMEMO, callcache)] ruby vm type:[T_IMEMO] thread id:[649074 (2660)] thread name:[main] allocation class:[(VM Internal, T_IMEMO, constcache)] ruby vm type:[T_IMEMO] thread id:[649074 (2660)] thread name:[main] allocation class:[(VM Internal, T_IMEMO, ment)] ruby vm type:[T_IMEMO] thread id:[649074 (2660)] thread name:[main] ``` whereas on Ruby 2.x, I got the `(VM Internal, T_IMEMO)` only.
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.
If we are not going to test it now, I think there should be a comment in code explaining this rationale.
I had an idea for a test that wasn't too boilerplaty/fragile, added in 02e3f10 . |
Something something about this being why it's important to always add test coverage, even if non-trivial to add.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3290 +/- ##
==========================================
- Coverage 98.22% 98.22% -0.01%
==========================================
Files 1253 1253
Lines 72545 72568 +23
Branches 3404 3406 +2
==========================================
+ Hits 71260 71282 +22
- Misses 1285 1286 +1 ☔ View full report in Codecov by Sentry. |
What does this PR do?
This PR extends the code that generates a description for an object during allocation to include the specific kind of
T_IMEMO
object.On Ruby < 3, the
rb_imemo_name
function we're relying on does not exist, so I chose to just not gather this information for legacy Rubies. (It's posssible to still get it, but I didn't want to spend more time).Motivation:
During discussion of #3282 the question of what a
T_IMEMO
is came up, and since it turns out it can represent different kinds of things in the Ruby VM, I decided to see if there was a way of spelling those out, and it turns out there is.Is this going to be useful for most customers? Probably not, but even for our internal debugging/learning, it may come in handy, so it was a bit of a "why not" kinda thing.
Additional Notes:
N/A
How to test the change?
I hesitated in terms of testing this, since these objects are internally created by the VM so we could only run a bit of code and kinda hope to trigger their creation.
Since this was a kind-of small thing I... decided to just leave it as-is without a specific test.
In the future, we may add some testing, if we think this feature is useful enough.
I did test it locally -- on Ruby 3.x I got from this trivial script
DD_PROFILING_ENABLED=true bundle exec ddtracerb exec ruby -e "def foo; end; foo; sleep(1)"
(output fromgo pprof tool
):whereas on Ruby 2.x, I got the
(VM Internal, T_IMEMO)
only.For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.