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-5942] Disallow sampling ractors in profiler #2357

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Nov 9, 2022

What does this PR do?:

This PR builds on top of #2350 and changes the Collectors::CpuAndWallTimeWorker to never trigger sampling if any of its methods get called from outside the main ractor.

This means that the profiler will not have visibility beyond the main ractor. Building support for that will come later, as we'll need to deal with the complexity of the profiler potentially being called from multiple ractors.

Motivation:

We don't yet support Ractors, but on an application using Ractors the profiler should not fail or misbehave. The expected behavior is that it just won't profile Ractors beyond the main one.

Additional Notes:

(Nothing)

How to test the change?:

I've included partial coverage, but testing the GC-related behavior was harder than I expected (also because of #2354), so for now I didn't add it.

To test this feature, you can enable profiling on a Ruby app using ractors and confirm that no samples appear from Ractors, but otherwise profiling works correctly (for threads in the main Ractor).

Here's a test app I've been using:

puts RUBY_DESCRIPTION

def fib(n)
  return n if n <= 1
  fib(n-1) + fib(n-2)
end

Thread.current.name = "#{Thread.current.object_id} main"

ractor1 = Ractor.new do
  Thread.current.name = "#{Thread.current.object_id} ractor1 main"

  Thread.new do
    Thread.current.name = "#{Thread.current.object_id} ractor1 background"

    loop { fib(40) }
  end

  sleep 1
  puts "ractor1 => #{Thread.list}"

  loop { fib(40) }
end

ractor2 = Ractor.new do
  Thread.current.name = "#{Thread.current.object_id} ractor2 main"

  Thread.new do
    Thread.current.name = "#{Thread.current.object_id} ractor2 background"

    loop { fib(40) }
  end

  sleep 1
  puts "ractor2 => #{Thread.list}"

  loop { fib(40) }
end

puts "main => #{Thread.list}"

sleep

**What does this PR do?**:

This PR builds on top of #2350 and changes the
`Collectors::CpuAndWallTimeWorker` to never trigger sampling if any
of its methods get called from outside the main ractor.

This means that the profiler will not have visibility beyond the
main ractor. Building support for that will come later, as we'll need
to deal with the complexity of the profiler potentially being called
from multiple ractors.

**Motivation**:

We don't yet support Ractors, but on an application using Ractors the
profiler should not fail (it just won't show the Ractors).

**Additional Notes**:

(Nothing)

**How to test the change?**:

I've included partial coverage, but testing the GC-related behavior
was harder than I expected, so for now I didn't add it.

To test this feature, you can enable profiling on a Ruby app using
ractors and confirm that no samples appear from Ractors, but otherwise
profiling works correctly (for threads in the main Ractor).

Here's a test app I've been using:

```ruby
puts RUBY_DESCRIPTION

def fib(n)
  return n if n <= 1
  fib(n-1) + fib(n-2)
end

Thread.current.name = "#{Thread.current.object_id} main"

ractor1 = Ractor.new do
  Thread.current.name = "#{Thread.current.object_id} ractor1 main"

  Thread.new do
    Thread.current.name = "#{Thread.current.object_id} ractor1 background"

    loop { fib(40) }
  end

  sleep 1
  puts "ractor1 => #{Thread.list}"

  loop { fib(40) }
end

ractor2 = Ractor.new do
  Thread.current.name = "#{Thread.current.object_id} ractor2 main"

  Thread.new do
    Thread.current.name = "#{Thread.current.object_id} ractor2 background"

    loop { fib(40) }
  end

  sleep 1
  puts "ractor2 => #{Thread.list}"

  loop { fib(40) }
end

puts "main => #{Thread.list}"

sleep
```
@ivoanjo ivoanjo requested a review from a team November 9, 2022 09:45
@github-actions github-actions bot added the profiling Involves Datadog profiling label Nov 9, 2022
Ran into <https://bugs.ruby-lang.org/issues/18464> in CI, so as a
workaround we need to make sure to not have GC profiling enabled when
Ractors are being created.
@github-actions github-actions bot added the core Involves Datadog core libraries label Nov 9, 2022
Copy link
Contributor

@lloeki lloeki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all seems very reasonable!

@ivoanjo ivoanjo merged commit 72a7907 into master Nov 10, 2022
@ivoanjo ivoanjo deleted the ivoanjo/prof-5942-ractor-support branch November 10, 2022 14:16
@github-actions github-actions bot added this to the 1.6.0 milestone Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants