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

[PROF-10987] Add Ruby GC guard to heap profiler as a preventative measure #4187

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Dec 3, 2024

What does this PR do?

This PR adds a RUBY_GC_GUARD call to the Ruby heap profiler, making sure that the object being pointed by the ref local variable does not get garbage collected too early.

Motivation:

This is a speculative fix:
We've seen a crash coming from calls to ruby_obj_memsize_of (line 654). Adding this RUBY_GC_GUARD is a very-low-cost "just in case" until we can fully get to the bottom of this crash, and even if it wasn't needed, it's very harmless to have.

Change log entry

(Internal change, not relevant for changelog)

Additional Notes:

N/A

How to test the change?

This is part of the challenge -- it's extremely hard to test this change, as in many compilers it may even turn out to be a no-op.

We'll need to wait for more info on the crash to go further than this change.

…sure

**What does this PR do?**

This PR adds a `RUBY_GC_GUARD` call to the Ruby heap profiler, making
sure that the object being pointed by the `ref` local variable does
not get garbage collected too early.

**Motivation:**

This is a speculative fix:
We've seen a crash coming from calls to `ruby_obj_memsize_of`
(line 654). Adding this `RUBY_GC_GUARD` is a very-low-cost
"just in case" until we can fully get to the bottom of this crash,
and even if it wasn't needed, it's very harmless to have.

**Additional Notes:**

N/A

**How to test the change?**

This is part of the challenge -- it's extremely hard to test this
change, as in many compilers it may even turn out to be a no-op.

We'll need to wait for more info on the crash to go further than
this change.
@ivoanjo ivoanjo requested review from a team as code owners December 3, 2024 17:04
@ivoanjo ivoanjo added profiling Involves Datadog profiling dev/internal Other internal work that does not need to be included in the changelog bug Involves a bug labels Dec 3, 2024
@datadog-datadog-prod-us1
Copy link
Contributor

Datadog Report

Branch report: ivoanjo/prof-10987-add-gc-guard
Commit report: a38bed3
Test service: dd-trace-rb

✅ 0 Failed, 22036 Passed, 1459 Skipped, 5m 11.27s Total Time
⌛ 3 Performance Regressions

⌛ Performance Regressions vs Default Branch (3)

  • Rails integration tests for an application with a basic route behaves like appsec standalone billing without upstream appsec propagation with attack and trace is kept with priority 2 from 0 sampling priority behaves like a trace sent to agent with Datadog-Client-Computed-Stats header is expected to equal true - rspec 3.63s (+3.03s, +503%) - Details
  • Rails integration tests for an application with a basic route behaves like appsec standalone billing without upstream appsec propagation with attack and trace is kept with priority 2 from 0 sampling priority behaves like a trace with ASM Standalone tags is expected to eq 1212121212121212121 - rspec 3.43s (+2.83s, +468%) - Details
  • Rails integration tests for an application with a basic route behaves like appsec standalone billing without upstream appsec propagation with attack and trace is kept with priority 2 from -1 sampling priority behaves like a trace with ASM Standalone tags is expected to eq 1212121212121212121 - rspec 3.45s (+2.85s, +468%) - Details

@pr-commenter
Copy link

pr-commenter bot commented Dec 3, 2024

Benchmarks

Benchmark execution time: 2024-12-03 17:27:57

Comparing candidate commit a38bed3 in PR branch ivoanjo/prof-10987-add-gc-guard with baseline commit 7f079c9 in branch master.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 30 metrics, 2 unstable metrics.

scenario:profiler - sample timeline=false

  • 🟩 throughput [+0.455op/s; +0.467op/s] or [+7.553%; +7.746%]

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.74%. Comparing base (7f079c9) to head (a38bed3).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4187      +/-   ##
==========================================
- Coverage   97.75%   97.74%   -0.01%     
==========================================
  Files        1357     1357              
  Lines       81890    81890              
  Branches     4164     4164              
==========================================
- Hits        80054    80046       -8     
- Misses       1836     1844       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ivoanjo ivoanjo enabled auto-merge December 4, 2024 09:49
@ivoanjo ivoanjo merged commit cfd35c7 into master Dec 4, 2024
174 of 194 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/prof-10987-add-gc-guard branch December 4, 2024 09:50
@github-actions github-actions bot added this to the 2.8.0 milestone Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug dev/internal Other internal work that does not need to be included in the changelog profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants