-
Notifications
You must be signed in to change notification settings - Fork 11
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
Optimize getting caller location using some ruby internals #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 👍 One question about ruby version support, and a couple of nitpicks. Otherwise LGTM.
ext/rotoscope/callsite.c
Outdated
size_t ruby_control_frame_size; | ||
|
||
// MRI ruby control frames are stored in an array as of | ||
// ruby 2.4.1, so we depend on this to be able to get the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grammar nit "we depend on this to be able to get the determine the control…"
ext/rotoscope/rotoscope.h
Outdated
@@ -3,6 +3,8 @@ | |||
|
|||
#include "unistd.h" | |||
|
|||
#include "callsite.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't need any of the types for rotoscope.h
does it make sense to move this include into rotoscope.c
? That's what I've done with the other includes but I don't know what's more standard (I'm aware they're functionally equivalent though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, I could move it into rotoscope.c
ext/rotoscope/callsite.c
Outdated
size_t ruby_control_frame_size; | ||
|
||
// MRI ruby control frames are stored in an array as of | ||
// ruby 2.4.1, so we depend on this to be able to get the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression we were able to successfully advance the frame pointer in 2.3.3 as well. Do we need a #ifdef
guard in this code for the ruby version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I didn't really say that properly. I meant to say that this is the behaviour at least until ruby 2.4.1, since I can't predict the future.
We want to get the caller's control frame path and line number in a trace event hook without the overhead of going through a method like `caller` or `caller_locations`. The ruby tracepoint API only gives access to the called functions path and line number. Instead, we rely on the minimum amount or ruby internals needed to get the caller's path and line number by changing the `cfp` field of the internal rb_trace_arg_struct before getting the path and lineno from the trace arg.
035d2fb
to
eaa80c7
Compare
Fixes #16
Problem
We want to get the caller's control frame path and line number in a trace event hook without the overhead of going through a method like
caller
orcaller_locations
. The ruby tracepoint API only gives access to the current frames path and line number.Solution
Get the caller frame's path and line number by modifying the internal
cfp
field of the trace arg to point to the caller's control frame. This means changes to this rb_trace_arg_struct struct in ruby could break the library.The caller's control frame can be found by adding the size of a control frame to the
cfp
. Therb_control_frame_t
is also internal to ruby, so we determine this by getting the difference between thecfp
for two nested function calls during initialization.Since we are depending on ruby internals, even a patch release of ruby could break this gem. However, I did minimize the amount of internals that this patch depends on to reduce the chance of it breaking in the future.