Skip to content
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

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Nov 29, 2023

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.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

**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.
@ivoanjo ivoanjo requested a review from a team as a code owner November 29, 2023 18:56
@github-actions github-actions bot added the profiling Involves Datadog profiling label Nov 29, 2023
Copy link
Member

@marcotc marcotc left a 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.

@ivoanjo
Copy link
Member Author

ivoanjo commented Nov 30, 2023

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-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2d4dad6) 98.22% compared to head (2ce6c63) 98.22%.

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.
📢 Have feedback on the report? Share it here.

@ivoanjo ivoanjo merged commit 96d4457 into master Nov 30, 2023
178 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/include-imemo-kind branch November 30, 2023 11:15
@github-actions github-actions bot added this to the 1.18.0 milestone Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants