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

Collect stack frames immediately in Ruby 3.0 #150

Merged
merged 3 commits into from Apr 29, 2021

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Apr 28, 2021

As of ruby/ruby@0e276dc, which shipped in Ruby 3.0, it seems to be safe to collect stack frames inside the signal handler. This should allow more accurate results than waiting for the postponed job to run since that can only measure when interrupts are checked.

This new behaviour is wrapped inside a preprocessor check for Ruby 3+

Additionally, this moves the "in signal handler" checks up a level, and uses pthread_mutex_trylock, which should be more reliable and I believe will fix the issue described in #123 and #124

@tenderlove and I had some discussion about whether this could have issues due to the writes in ruby/ruby@0e276dc being reordered.

One concern is whether there could be issues with hardware memory reordering (particularly on arm). I believe this is safe only because we are only considering the stack from our current thread. These writes will appear consistent in our interrupt handler because of this.

Another concern is that the compiler could reorder the writes in ruby/ruby@0e276dc. It doesn't seem to be, but that could absolutely happen. I think we should investigate making a change to Ruby to ensure the writes in vm_push_frame/vm_pop_frame aren't reordered.

jhawthorn and others added 3 commits April 28, 2021 13:23
Because this is in the postponed job handler, we should never be able to
reenter this code.

I also believe this has a race condition if another signal arrives
between the if statement and the increment. Signals can also arrive in
any thread so this would not be a safe way to avoid reentrancy.
This entures we won't re-enter the signal handler from another signal
whether or not it happens in our own thread.
Co-authored-by: Aaron Patterson <aaron.patterson@gmail.com>
@eregon
Copy link
Contributor

eregon commented Aug 22, 2022

One concern is whether there could be issues with hardware memory reordering (particularly on arm). I believe this is safe only because we are only considering the stack from our current thread. These writes will appear consistent in our interrupt handler because of this.

I think being in a signal handler gives up on the the "golden rule of multithreading": "a single thread should see everything it does as if it was done in that order".
My understanding is a signal handler is basically considered concurrency/like another thread in C.

AFAIK the signal could trigger anywhere, e.g. between two writes on vm_push_frame (even if they are in order, a big if) and that could cause issues, isn't it?

@ivoanjo
Copy link
Contributor

ivoanjo commented Aug 22, 2022

AFAIK the signal could trigger anywhere, e.g. between two writes on vm_push_frame (even if they are in order, a big if) and that could cause issues, isn't it?

+1 I planned experimenting with this and raising it at some point -- the change in ruby/ruby@0e276dc did reorder things as far as the C source goes, but as far as I see it there really doesn't seem to be anything guaranteeing that the compiler won't reorder the write to ec->cfp with the actual initialization of the structure.

So... yeah this doesn't seem particularly safe at this moment.

(But it would be great if rb_profile_frames could indeed be made async-safe!)

@mame
Copy link
Contributor

mame commented Nov 16, 2022

As @eregon and @ivoanjo have said, I think there is a theoretical concern with this change.

However, it may not lead to a visible error on major architectures and C compilers. Even in a potentially reproducible environment, it will be very unlikely to be visible unless the signal handler is called at an extremely unlucky timing.

I think it is a possible option to ignore such a theoretical concern and take the practical benefits, but it's a difficult decision. I don't interfere with your decisions in stackprof, but if it were the interpreter itself, I would like to be very cautious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants