Skip to content

Add fake stack frames representing samples taken during garbage collection #87

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

Merged
merged 7 commits into from
Nov 17, 2017
Merged

Add fake stack frames representing samples taken during garbage collection #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
df48175
Add fake stack frames representing samples taken during garbage colle…
Nov 15, 2017
7f27c52
Mark variables as global to prevent GC, allocate up front
Nov 16, 2017
6cdfe72
Add back accidentally removed line
Nov 16, 2017
6c3fefa
Record GC sample outside of signal handler
Nov 16, 2017
53379fc
Ensure we allocate enough memory to cover the samples
Nov 16, 2017
0c71571
Use a separate job handler for gc
Nov 17, 2017
7653803
Use rb_str_new_cstr instead of rb_str_new_literal
Nov 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 67 additions & 15 deletions ext/stackprof/stackprof.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,12 @@ static struct {
size_t overall_signals;
size_t overall_samples;
size_t during_gc;
size_t unrecorded_gc_samples;
st_table *frames;

VALUE fake_gc_frame;
VALUE fake_gc_frame_name;
VALUE empty_string;
VALUE frames_buffer[BUF_SIZE];
int lines_buffer[BUF_SIZE];
} _stackprof;
Expand Down Expand Up @@ -187,16 +191,24 @@ frame_i(st_data_t key, st_data_t val, st_data_t arg)

rb_hash_aset(results, rb_obj_id(frame), details);

name = rb_profile_frame_full_label(frame);
rb_hash_aset(details, sym_name, name);
if (frame == _stackprof.fake_gc_frame) {
name = _stackprof.fake_gc_frame_name;
file = _stackprof.empty_string;
line = INT2FIX(0);
} else {
name = rb_profile_frame_full_label(frame);

file = rb_profile_frame_absolute_path(frame);
if (NIL_P(file))
file = rb_profile_frame_path(frame);
rb_hash_aset(details, sym_file, file);
file = rb_profile_frame_absolute_path(frame);
if (NIL_P(file))
file = rb_profile_frame_path(frame);
line = rb_profile_frame_first_lineno(frame);
}

if ((line = rb_profile_frame_first_lineno(frame)) != INT2FIX(0))
rb_hash_aset(details, sym_name, name);
rb_hash_aset(details, sym_file, file);
if (line != INT2FIX(0)) {
rb_hash_aset(details, sym_line, line);
}

rb_hash_aset(details, sym_total_samples, SIZET2NUM(frame_data->total_samples));
rb_hash_aset(details, sym_samples, SIZET2NUM(frame_data->caller_samples));
Expand Down Expand Up @@ -340,13 +352,12 @@ st_numtable_increment(st_table *table, st_data_t key, size_t increment)
}

void
stackprof_record_sample()
stackprof_record_sample_for_stack(int num)
{
int num, i, n;
int i, n;
VALUE prev_frame = Qnil;

_stackprof.overall_samples++;
num = rb_profile_frames(0, sizeof(_stackprof.frames_buffer) / sizeof(VALUE), _stackprof.frames_buffer, _stackprof.lines_buffer);

if (_stackprof.raw) {
int found = 0;
Expand All @@ -356,7 +367,7 @@ stackprof_record_sample()
_stackprof.raw_samples = malloc(sizeof(VALUE) * _stackprof.raw_samples_capa);
}

if (_stackprof.raw_samples_capa <= _stackprof.raw_samples_len + num) {
while (_stackprof.raw_samples_capa <= _stackprof.raw_samples_len + (num + 2)) {
Copy link
Collaborator Author

@jlfwong jlfwong Nov 16, 2017

Choose a reason for hiding this comment

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

Okay! I think this fixes it.

I believe this code was previously unsafe.

This is checking to see if the capacity of raw_samples is sufficient to cover num new entries, but it really needs to cover up to num + 2 new entries, since each sample entry is

stack height, followed by n frame ids, followed by the sample count.

I think this was an issue even before this change, but I'm not able to reproduce a crash before this PR.

Regardless, I think this is safer now?

Copy link
Owner

@tmm1 tmm1 Nov 16, 2017

Choose a reason for hiding this comment

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

I'm not sure I follow.. why would this need to be a loop?

Copy link
Owner

@tmm1 tmm1 Nov 16, 2017

Choose a reason for hiding this comment

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

I see, because we just double every time.

Calling realloc multiple times seems silly. It should calculate the required size once, and then call realloc once.

Copy link
Collaborator Author

@jlfwong jlfwong Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

In practice, I suspect this will almost never need to call realloc more than once, because we set the initial buffer size to be 100 * the number of frames in the first sample, and for this to be called more than once, a given stack would have to be more than the size of all previous stacks combined. The while loop is just for safety, but I suspect in practice it'll never be a significant performance burden.

Copy link
Owner

@tmm1 tmm1 Nov 16, 2017

Choose a reason for hiding this comment

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

Okay fair enough.

_stackprof.raw_samples_capa *= 2;
_stackprof.raw_samples = realloc(_stackprof.raw_samples, sizeof(VALUE) * _stackprof.raw_samples_capa);
}
Expand Down Expand Up @@ -411,6 +422,39 @@ stackprof_record_sample()
}
}

void
stackprof_record_sample()
{
int num = rb_profile_frames(0, sizeof(_stackprof.frames_buffer) / sizeof(VALUE), _stackprof.frames_buffer, _stackprof.lines_buffer);
stackprof_record_sample_for_stack(num);
}

void
stackprof_record_gc_samples()
{
int i;

_stackprof.frames_buffer[0] = _stackprof.fake_gc_frame;
_stackprof.lines_buffer[0] = 0;
for (i = 0; i < _stackprof.unrecorded_gc_samples; i++) {
stackprof_record_sample_for_stack(1);
}
_stackprof.during_gc += _stackprof.unrecorded_gc_samples;
_stackprof.unrecorded_gc_samples = 0;
}

static void
stackprof_gc_job_handler(void *data)
{
static int in_signal_handler = 0;
if (in_signal_handler) return;
if (!_stackprof.running) return;

in_signal_handler++;
stackprof_record_gc_samples();
in_signal_handler--;
}

static void
stackprof_job_handler(void *data)
{
Expand All @@ -427,10 +471,12 @@ static void
stackprof_signal_handler(int sig, siginfo_t *sinfo, void *ucontext)
{
_stackprof.overall_signals++;
if (rb_during_gc())
_stackprof.during_gc++, _stackprof.overall_samples++;
else
rb_postponed_job_register_one(0, stackprof_job_handler, 0);
if (rb_during_gc()) {
_stackprof.unrecorded_gc_samples++;
rb_postponed_job_register_one(0, stackprof_gc_job_handler, (void*)0);
} else {
rb_postponed_job_register_one(0, stackprof_job_handler, (void*)0);
}
}

static void
Expand Down Expand Up @@ -532,6 +578,12 @@ Init_stackprof(void)
gc_hook = Data_Wrap_Struct(rb_cObject, stackprof_gc_mark, NULL, &_stackprof);
rb_global_variable(&gc_hook);

_stackprof.fake_gc_frame = INT2FIX(0x9C);
_stackprof.empty_string = rb_str_new_cstr("");
_stackprof.fake_gc_frame_name = rb_str_new_cstr("(garbage collection)");
rb_global_variable(&_stackprof.fake_gc_frame_name);
rb_global_variable(&_stackprof.empty_string);

rb_mStackProf = rb_define_module("StackProf");
rb_define_singleton_method(rb_mStackProf, "running?", stackprof_running_p, 0);
rb_define_singleton_method(rb_mStackProf, "run", stackprof_run, -1);
Expand Down
5 changes: 3 additions & 2 deletions test/test_stackprof.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,14 @@ def test_fork
end

def test_gc
profile = StackProf.run(interval: 100) do
profile = StackProf.run(interval: 100, raw: true) do
5.times do
GC.start
end
end

assert_empty profile[:frames]
raw = profile[:raw]
assert_equal profile[:frames][raw[1]][:name], '(garbage collection)'
assert_operator profile[:gc_samples], :>, 0
assert_equal 0, profile[:missed_samples]
end
Expand Down