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.

Sign up for GitHub

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

Jump to bottom

Correctly locate filepath of caller to tracepoint #13

Merged
merged 2 commits into from
Mar 21, 2017
Merged

Correctly locate filepath of caller to tracepoint #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c42c834
[Perf Regression] Look up filepath of call via VM backtrace
jahfer Feb 10, 2017
1a76b68
Changes based on review
jahfer Mar 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
99 changes: 78 additions & 21 deletions ext/rotoscope/rotoscope.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "zlib.h"

VALUE cRotoscope;

// recursive with singleton2str
static rs_class_desc_t class2str(VALUE klass);

Expand Down Expand Up @@ -45,19 +46,28 @@ static bool rejected_path(const char *path, Rotoscope *config)
return false;
}

static VALUE class_of_singleton(VALUE klass)
{
return rb_iv_get(klass, "__attached__");
}

static bool is_class_singleton(VALUE klass)
{
VALUE obj = class_of_singleton(klass);
return (RB_TYPE_P(obj, T_MODULE) || RB_TYPE_P(obj, T_CLASS));
}

static char *singleton2str(VALUE klass)
{
VALUE obj = rb_iv_get(klass, "__attached__");
if (RB_TYPE_P(obj, T_MODULE) || RB_TYPE_P(obj, T_CLASS))
if (is_class_singleton(klass))
{
// singleton of a class
VALUE obj = class_of_singleton(klass);
VALUE cached_lookup = rb_class_path_cached(obj);
VALUE name = (NIL_P(cached_lookup)) ? rb_class_name(obj) : cached_lookup;
return RSTRING_PTR(name);
}
else
else // singleton of an instance
{
// singleton of an instance
VALUE real_klass;
VALUE ancestors = rb_mod_ancestors(klass);
if (RARRAY_LEN(ancestors) > 0 && !NIL_P(real_klass = rb_ary_entry(ancestors, 1)))
Expand Down Expand Up @@ -92,8 +102,11 @@ static rs_class_desc_t class2str(VALUE klass)
{
if (FL_TEST(klass, FL_SINGLETON))
{
real_class.method_level = SINGLETON_METHOD;
real_class.name = singleton2str(klass);
if (is_class_singleton(klass))
{
real_class.method_level = CLASS_METHOD;
}
}
else
{
Expand All @@ -104,10 +117,44 @@ static rs_class_desc_t class2str(VALUE klass)
return real_class;
}

static const char *tracearg_path(rb_trace_arg_t *trace_arg)
static char *caller_location(int start)
{
VALUE path = rb_tracearg_path(trace_arg);
return RTEST(path) ? RSTRING_PTR(path) : "";
VALUE argv[2] = {INT2NUM(start), INT2NUM(1)};
VALUE res = rb_funcallv(rb_mKernel, rb_intern("caller_locations"), 2, argv);
return RSTRING_PTR(rb_inspect(rb_ary_entry(res, 0)));
}

static rs_callsite_t tracearg_path(rb_trace_arg_t *trace_arg)
{
rb_event_flag_t event_flag = rb_tracearg_event_flag(trace_arg);
rs_callsite_t callsite;
callsite.lineno = 0;
VALUE path;
char *caller_str = NULL;

switch (event_flag)
{
case RUBY_EVENT_C_CALL:
path = rb_tracearg_path(trace_arg);
strncpy((char *)callsite.filepath, RTEST(path) ? RSTRING_PTR(path) : "", sizeof(callsite.filepath));
callsite.lineno = FIX2INT(rb_tracearg_lineno(trace_arg));
break;
case RUBY_EVENT_C_RETURN:
caller_str = caller_location(0) + 1; // +1 to drop opening quote
// drop down to default on purpose
default:
if (caller_str || (caller_str = (caller_location(1) + 1)))
{
strncpy((char *)callsite.filepath, caller_str, sizeof(callsite.filepath));
strtok((char *)callsite.filepath, ":");
if (caller_str = strtok(NULL, ":"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

after lots of back-and-forth here, I've settled on two invocations of strtok instead of strchr since there's some extra bounds checking done by strtok that I don't want to have to reimplement.

tl;dr I got a segfault and gave up on strchr

{
callsite.lineno = (unsigned int)strtoul(caller_str, NULL, 10);
}
}
}

return callsite;
}

static rs_class_desc_t tracearg_class(rb_trace_arg_t *trace_arg)
Expand All @@ -127,17 +174,23 @@ static rs_class_desc_t tracearg_class(rb_trace_arg_t *trace_arg)
return class2str(klass);
}

static rs_tracepoint_t extract_full_tracevals(rb_trace_arg_t *trace_arg)
static char *tracearg_method_name(rb_trace_arg_t *trace_arg)
{
return RSTRING_PTR(rb_sym2str(rb_tracearg_method_id(trace_arg)));
}

static rs_tracepoint_t extract_full_tracevals(rb_trace_arg_t *trace_arg, const rs_callsite_t *callsite)
{
rs_class_desc_t method_owner = tracearg_class(trace_arg);
rb_event_flag_t event_flag = rb_tracearg_event_flag(trace_arg);

return (rs_tracepoint_t){
.event = evflag2name(rb_tracearg_event_flag(trace_arg)),
.event = evflag2name(event_flag),
.entity = method_owner.name,
.method_name = RSTRING_PTR(rb_sym2str(rb_tracearg_method_id(trace_arg))),
.filepath = tracearg_path(trace_arg),
.lineno = FIX2INT(rb_tracearg_lineno(trace_arg)),
.method_level = method_owner.method_level};
.method_name = tracearg_method_name(trace_arg),
.method_level = method_owner.method_level,
.filepath = callsite->filepath,
.lineno = callsite->lineno};
}

static bool in_fork(Rotoscope *config)
Expand All @@ -149,26 +202,28 @@ static void event_hook(VALUE tpval, void *data)
{
Rotoscope *config = (Rotoscope *)data;

if (in_fork(config)) {
if (in_fork(config))
{
rb_tracepoint_disable(config->tracepoint);
return;
}

rb_trace_arg_t *trace_arg = rb_tracearg_from_tracepoint(tpval);
const char *trace_path = tracearg_path(trace_arg);
rs_callsite_t trace_path = tracearg_path(trace_arg);

if (rejected_path(trace_path, config))
if (rejected_path(trace_path.filepath, config))
return;

rs_tracepoint_t trace = extract_full_tracevals(trace_arg);
rs_tracepoint_t trace = extract_full_tracevals(trace_arg, &trace_path);
gzprintf(config->log, RS_CSV_FORMAT, RS_CSV_VALUES(trace));
}

static void close_gz_handle(Rotoscope *config)
{
if (config->log)
{
if (!in_fork(config)) {
if (!in_fork(config))
{
gzclose(config->log);
}
config->log = NULL;
Expand Down Expand Up @@ -241,13 +296,15 @@ VALUE rotoscope_stop_trace(VALUE self)
{
rb_tracepoint_disable(config->tracepoint);
}

return Qnil;
}

VALUE rotoscope_mark(VALUE self)
{
Rotoscope *config = get_config(self);
if (!in_fork(config)) {
if (!in_fork(config))
{
gzprintf(config->log, "---\n");
}
return Qnil;
Expand Down
10 changes: 9 additions & 1 deletion ext/rotoscope/rotoscope.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@
#define RS_CSV_HEADER "event,entity,method_name,method_level,filepath,lineno\n"
#define RS_CSV_FORMAT "%s,\"%s\",\"%s\",%s,\"%s\",%d\n"

#define SINGLETON_METHOD "singleton"
#define CLASS_METHOD "class"
#define INSTANCE_METHOD "instance"

#define UNKNOWN_FILE_PATH "Unknown"

typedef struct
{
const char *event;
Expand Down Expand Up @@ -40,4 +42,10 @@ typedef struct
const char *method_level;
} rs_class_desc_t;

typedef struct
{
const char filepath[200];
unsigned int lineno;
} rs_callsite_t;

#endif
10 changes: 10 additions & 0 deletions test/fixture_inner.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true
class FixtureInner
def do_work
raise unless sum == 2
end

def sum
1 + 1
end
end
10 changes: 10 additions & 0 deletions test/fixture_outer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true
class FixtureOuter
def initialize
@inner = FixtureInner.new
end

def do_work
@inner.do_work
end
end
52 changes: 38 additions & 14 deletions test/rotoscope_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
# frozen_string_literal: true
$LOAD_PATH.unshift File.expand_path('../../lib', __FILE__)
$LOAD_PATH.unshift File.expand_path('../', __FILE__)
require 'rotoscope'
require 'minitest/autorun'
require 'zlib'
require 'fileutils'
require 'csv'

require 'fixture_inner'
require 'fixture_outer'

class Example
class << self
def singleton_method
Expand All @@ -31,23 +35,25 @@ def oops
end

ROOT_FIXTURE_PATH = File.expand_path('../', __FILE__)
INNER_FIXTURE_PATH = File.expand_path('../fixture_inner.rb', __FILE__)
OUTER_FIXTURE_PATH = File.expand_path('../fixture_outer.rb', __FILE__)

class RotoscopeTest < MiniTest::Test
def setup
@logfile = File.expand_path('tmp/test.csv.gz')
end

def teardown
# FileUtils.remove_file(@logfile)
FileUtils.remove_file(@logfile)
end

def test_instance_method
contents = rotoscope_trace { Example.new.normal_method }
assert_equal [
{ event: "call", entity: "Example", method_name: "new", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "new", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "initialize", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "initialize", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "new", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "new", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "normal_method", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "normal_method", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 }
], parse_and_normalize(contents)
Expand All @@ -63,8 +69,8 @@ def test_calls_are_consistent_after_exception
def test_formats_singletons_of_a_class
contents = rotoscope_trace { Example.singleton_method }
assert_equal [
{ event: "call", entity: "Example", method_name: "singleton_method", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "singleton_method", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 }
{ event: "call", entity: "Example", method_name: "singleton_method", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "singleton_method", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 }
], parse_and_normalize(contents)

assert_frames_consistent contents
Expand All @@ -73,14 +79,32 @@ def test_formats_singletons_of_a_class
def test_formats_singletons_of_an_instance
contents = rotoscope_trace { Example.new.singleton_class.singleton_method }
assert_equal [
{ event: "call", entity: "Example", method_name: "new", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "new", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "initialize", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "initialize", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "new", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "new", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "singleton_class", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "singleton_class", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "singleton_method", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "singleton_method", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "singleton_class", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "singleton_method", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "singleton_method", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
], parse_and_normalize(contents)

assert_frames_consistent contents
end

def test_ignores_calls_if_blacklisted
contents = rotoscope_trace([INNER_FIXTURE_PATH, OUTER_FIXTURE_PATH]) do
foo = FixtureOuter.new
foo.do_work
end

assert_equal [
{ event: "call", entity: "FixtureOuter", method_name: "new", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "FixtureOuter", method_name: "initialize", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "FixtureOuter", method_name: "initialize", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "FixtureOuter", method_name: "new", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "FixtureOuter", method_name: "do_work", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "FixtureOuter", method_name: "do_work", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
], parse_and_normalize(contents)

assert_frames_consistent contents
Expand All @@ -99,10 +123,10 @@ def test_ignore_writes_in_fork
assert_equal [
{ event: "call", entity: "RotoscopeTest", method_name: "fork", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "RotoscopeTest", method_name: "fork", method_level: "instance", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "singleton_method", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "singleton_method", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Process", method_name: "wait", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Process", method_name: "wait", method_level: "singleton", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Example", method_name: "singleton_method", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Example", method_name: "singleton_method", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "call", entity: "Process", method_name: "wait", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
{ event: "return", entity: "Process", method_name: "wait", method_level: "class", filepath: "/rotoscope_test.rb", lineno: -1 },
], parse_and_normalize(contents)
end

Expand Down