Add profiler helper for detecting if we're on the main Ractor #2350
Conversation
**What does this PR do?**: Add a new helper to `private_vm_access.c` that allows the caller thread to ask if it's running as part of the main Ractor. On Rubies without Ractor support, as a simplification, we always return `true` so that callers don't need to probe for Ractor support. **Motivation**: This helper will be used to make sure the profiler code does not run on the wrong Ractor, since the profiler code is currently not built to be called in parallel from multiple Ractors. **Additional Notes**: Hey, I added the first spec that uses Ractors to our test suite! **How to test the change?**: Test coverage included.
See comment for more details.
ivoanjo
changed the title
|
Note: I may have found another Ruby bug (or at least something strange) that happens after the first Ractor gets created in a Ruby app. I suspect this PR will cause flaky tests because of that, so I'm moving it back to draft until I can ensure this will not be the case. |
|
Update: I was able to track down the flake issue to a Ruby VM bug, which I've reported upstream https://bugs.ruby-lang.org/issues/19112 . I've also deployed a workaround in #2354, and changed this PR to be on top of that one, to underline the implicit dependency -- the workaround needs to go in first so as to avoid introducing a flaky test. Marking this PR as "ready for review" once more. |
ivoanjo
changed the base branch from
master
to
ivoanjo/workaround-ractor-tracepoint-issue
| @@ -125,6 +125,9 @@ def add_compiler_flag(flag) | |||
| # On older Rubies, we need to use a backported version of this function. See private_vm_api_access.h for details. | |||
| $defs << '-DUSE_BACKPORTED_RB_PROFILE_FRAME_METHOD_NAME' if RUBY_VERSION < '3' | |||
|
|
|||
| # On older Rubies, there are no Ractors | |||
| $defs << '-DNO_RACTORS' if RUBY_VERSION < '3' | |||
There was a problem hiding this comment.
There's no built in define around this? Interesting...
There was a problem hiding this comment.
I did a quick search and there isn't any -- the closest I found has HAVE_RUBY_RACTOR_H which can maybe thought of as a way of doing that but... I decided to keep this one for now ;) (Unless you think it's a really bad idea)
I think the reason why there is no define is because you can't disable Ractors (that I know of), and thus you can always probe them by looking at the Ruby version, so that's probably why they don't have a more specific #define.
On our side, as you can kinda tell by this file, I've been somewhat avoiding doing something like $defs << "-DRUBY_#{RUBY_VERSION}" and then doing #ifdef RUBY_3.1 in the code, in favor of being a bit more semantic -- e.g. rather than saying "this is the codepath for Ruby 3.1", say "this is the codepath for when this structure has a member called foo".
| // This API and definition are exported as a public symbol by the VM BUT the function header is not defined in any public header, so we | ||
| // repeat it here to be able to use in our code. |
There was a problem hiding this comment.
Does it make sense to request/perform this change upstream?
There was a problem hiding this comment.
I think this "public-ish" is actually on purpose. That is, the missing header is not a "oops we forgot", it's a "this isn't really a public API, but some things outside of Ruby make good use of it, and we don't mind exposing it for now but you're on your own".
For instance in ruby/ruby@15476c6 you see a similar API that was published so that fiddle could use it:
(Out of curiosity, there was some openness during a few chats with core team members recently to perhaps promote a few more APIs that would be really useful for profilers to this "public-ish" state).
There was a problem hiding this comment.
But... maybe I'm just assuming too much. I've been hesitating a bit on asking for these kinds of things from upstream because I wanted to first come up with our most important asks and then go down the list from there, rather than asking for all of them.
**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 ```
What does this PR do?:
Add a new helper to
private_vm_access.cthat allows the caller thread to ask if it's running as part of the main Ractor.On Rubies without Ractor support, as a simplification, we always return
trueso that callers don't need to probe for Ractor support.Motivation:
This helper will be used to make sure the profiler code does not run on the wrong Ractor, since the profiler code is currently not built to be called in parallel from multiple Ractors.
Additional Notes:
Hey, I added the first spec that uses Ractors to our test suite!
How to test the change?:
Test coverage included.