Use more accurate profiling on Ruby 3.1 and fix async-signal-safety#172
Merged
tenderlove merged 2 commits intotmm1:masterfrom Feb 16, 2022
Merged
Use more accurate profiling on Ruby 3.1 and fix async-signal-safety#172tenderlove merged 2 commits intotmm1:masterfrom
tenderlove merged 2 commits intotmm1:masterfrom
Conversation
This is necessary for RUBY_API_VERSION_MAJOR, at least in Ruby 3.1
tenderlove
approved these changes
Feb 16, 2022
Collaborator
tenderlove
left a comment
There was a problem hiding this comment.
I added a couple comments, but the overall approach makes sense to me.
In 6c3f4d7 we started recording signals immediately on receiving a signal, after observing that in Ruby 3.0+ the CFP structure was safe to read. This resulted in much more accurate measurements. Unfortunately though rb_profile_frames seems to be safe to call inside a signal handler, stackprof_record_sample_for_stack is not, since it calls malloc (itself or through st_*) which is not async-signal-safe. This could potentially result in deadlocks or memory corruption depending on the malloc implementation. This commit attempts to restore the safety of the postponed job approach while keeping the accuracy of measuring immediately. Inside the signal handler we record the frames to a buffer, then enqueue a postponed job to do the actual recording.
0fa6a43 to
7a8e4ca
Compare
Contributor
|
Quick heads up, while testing ruby/ruby#6572, I got a VM crash in stackprof: I haven't investigated it much just yet, but I suspect ruby/ruby#6572 might break the async-signal safety assumption? |
Contributor
|
Also note that the crash only happens in CPU mode, WALL mode still work fine. |
Contributor
Author
|
I would be surprised to see async-signal-safety related to the crash you saw, since that PR is also using |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Accuracy Regression
In Ruby 3.1, stackprof was building with the warning:
Because of this, stackprof wasn't using the much improved accuracy added in #150 for Ruby 3.0+. This is fixed by adding an include of
ruby/version.hAsync signal safety
Revisiting #150, I noticed that approach wasn't quite safe 😅.
Unfortunately though rb_profile_frames seems to be safe to call inside a signal handler, stackprof_record_sample_for_stack is not, since it calls malloc (itself or through st_*) which is not async-signal-safe. This could potentially result in deadlocks or memory corruption depending on the malloc implementation.
This commit attempts to restore the safety of the postponed job approach while keeping the accuracy of measuring immediately. Inside the signal handler we record the frames to a buffer, then enqueue a postponed job to do the actual recording.
I am interested in further improving this by adding a worker thread to do the processing of the buffer instead of doing it in the postponed job, however I'd like to apply this as a smaller fix first and attempt that in a future PR.