From df48175fbcdf84bbb11176fb44757f0cdfe08ab1 Mon Sep 17 00:00:00 2001 From: Jamie Wong Date: Wed, 15 Nov 2017 15:58:44 -0800 Subject: [PATCH 1/7] Add fake stack frames representing samples taken during garbage collections --- ext/stackprof/stackprof.c | 47 ++++++++++++++++++++++++++++----------- test/test_stackprof.rb | 5 +++-- 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/ext/stackprof/stackprof.c b/ext/stackprof/stackprof.c index e1c517e3..b2c7680e 100644 --- a/ext/stackprof/stackprof.c +++ b/ext/stackprof/stackprof.c @@ -43,6 +43,7 @@ static struct { size_t during_gc; st_table *frames; + VALUE fake_gc_frame; VALUE frames_buffer[BUF_SIZE]; int lines_buffer[BUF_SIZE]; } _stackprof; @@ -187,16 +188,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 = rb_str_new_literal("(garbage collection)"); + file = rb_str_new_literal(""); + 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)); @@ -340,13 +349,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; @@ -411,6 +419,14 @@ 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); +} + + static void stackprof_job_handler(void *data) { @@ -427,10 +443,14 @@ 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 + if (rb_during_gc()) { + _stackprof.during_gc++; + _stackprof.frames_buffer[0] = _stackprof.fake_gc_frame; + _stackprof.lines_buffer[0] = -1; + stackprof_record_sample_for_stack(1); + } else { rb_postponed_job_register_one(0, stackprof_job_handler, 0); + } } static void @@ -530,6 +550,7 @@ Init_stackprof(void) #undef S gc_hook = Data_Wrap_Struct(rb_cObject, stackprof_gc_mark, NULL, &_stackprof); + _stackprof.fake_gc_frame = rb_str_new_literal("fake_gc_frame"); rb_global_variable(&gc_hook); rb_mStackProf = rb_define_module("StackProf"); diff --git a/test/test_stackprof.rb b/test/test_stackprof.rb index e09147d9..8b4ef2e9 100644 --- a/test/test_stackprof.rb +++ b/test/test_stackprof.rb @@ -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 From 7f27c5253692c3da324ebbeabf5851f04b8bb24a Mon Sep 17 00:00:00 2001 From: Jamie Wong Date: Wed, 15 Nov 2017 16:22:26 -0800 Subject: [PATCH 2/7] Mark variables as global to prevent GC, allocate up front --- ext/stackprof/stackprof.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/ext/stackprof/stackprof.c b/ext/stackprof/stackprof.c index b2c7680e..e7a4f91e 100644 --- a/ext/stackprof/stackprof.c +++ b/ext/stackprof/stackprof.c @@ -44,6 +44,8 @@ static struct { 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; @@ -189,8 +191,8 @@ frame_i(st_data_t key, st_data_t val, st_data_t arg) rb_hash_aset(results, rb_obj_id(frame), details); if (frame == _stackprof.fake_gc_frame) { - name = rb_str_new_literal("(garbage collection)"); - file = rb_str_new_literal(""); + name = _stackprof.fake_gc_frame_name; + file = _stackprof.empty_string; line = INT2FIX(0); } else { name = rb_profile_frame_full_label(frame); @@ -550,8 +552,13 @@ Init_stackprof(void) #undef S gc_hook = Data_Wrap_Struct(rb_cObject, stackprof_gc_mark, NULL, &_stackprof); + _stackprof.fake_gc_frame = rb_str_new_literal("fake_gc_frame"); - rb_global_variable(&gc_hook); + _stackprof.empty_string = rb_str_new_literal(""); + _stackprof.fake_gc_frame_name = rb_str_new_literal("(garbage collection)"); + rb_global_variable(&_stackprof.fake_gc_frame); + 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); From 6cdfe725fb795c5cb7dcaa468cf75830b3e84e03 Mon Sep 17 00:00:00 2001 From: Jamie Wong Date: Wed, 15 Nov 2017 16:49:33 -0800 Subject: [PATCH 3/7] Add back accidentally removed line --- ext/stackprof/stackprof.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/stackprof/stackprof.c b/ext/stackprof/stackprof.c index e7a4f91e..b227d3af 100644 --- a/ext/stackprof/stackprof.c +++ b/ext/stackprof/stackprof.c @@ -552,6 +552,7 @@ Init_stackprof(void) #undef S gc_hook = Data_Wrap_Struct(rb_cObject, stackprof_gc_mark, NULL, &_stackprof); + rb_global_variable(&gc_hook); _stackprof.fake_gc_frame = rb_str_new_literal("fake_gc_frame"); _stackprof.empty_string = rb_str_new_literal(""); From 6c3fefa3460dd57b1ab93273ef9603d0fb958045 Mon Sep 17 00:00:00 2001 From: Jamie Wong Date: Thu, 16 Nov 2017 10:17:07 -0800 Subject: [PATCH 4/7] Record GC sample outside of signal handler --- ext/stackprof/stackprof.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/ext/stackprof/stackprof.c b/ext/stackprof/stackprof.c index b227d3af..6743ba1d 100644 --- a/ext/stackprof/stackprof.c +++ b/ext/stackprof/stackprof.c @@ -428,16 +428,27 @@ stackprof_record_sample() stackprof_record_sample_for_stack(num); } +void +stackprof_record_gc_sample() +{ + _stackprof.frames_buffer[0] = _stackprof.fake_gc_frame; + _stackprof.lines_buffer[0] = 0; + stackprof_record_sample_for_stack(1); +} static void -stackprof_job_handler(void *data) +stackprof_job_handler(void *is_gc) { static int in_signal_handler = 0; if (in_signal_handler) return; if (!_stackprof.running) return; in_signal_handler++; - stackprof_record_sample(); + if ((int)is_gc) { + stackprof_record_gc_sample(); + } else { + stackprof_record_sample(); + } in_signal_handler--; } @@ -447,11 +458,9 @@ stackprof_signal_handler(int sig, siginfo_t *sinfo, void *ucontext) _stackprof.overall_signals++; if (rb_during_gc()) { _stackprof.during_gc++; - _stackprof.frames_buffer[0] = _stackprof.fake_gc_frame; - _stackprof.lines_buffer[0] = -1; - stackprof_record_sample_for_stack(1); + rb_postponed_job_register_one(0, stackprof_job_handler, (void*)1); } else { - rb_postponed_job_register_one(0, stackprof_job_handler, 0); + rb_postponed_job_register_one(0, stackprof_job_handler, (void*)0); } } @@ -554,10 +563,9 @@ Init_stackprof(void) gc_hook = Data_Wrap_Struct(rb_cObject, stackprof_gc_mark, NULL, &_stackprof); rb_global_variable(&gc_hook); - _stackprof.fake_gc_frame = rb_str_new_literal("fake_gc_frame"); + _stackprof.fake_gc_frame = INT2FIX(0x9C); _stackprof.empty_string = rb_str_new_literal(""); _stackprof.fake_gc_frame_name = rb_str_new_literal("(garbage collection)"); - rb_global_variable(&_stackprof.fake_gc_frame); rb_global_variable(&_stackprof.fake_gc_frame_name); rb_global_variable(&_stackprof.empty_string); From 53379fc946a3366e7c0afe6615c9439f5038ffd8 Mon Sep 17 00:00:00 2001 From: Jamie Wong Date: Thu, 16 Nov 2017 10:26:17 -0800 Subject: [PATCH 5/7] Ensure we allocate enough memory to cover the samples --- ext/stackprof/stackprof.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/stackprof/stackprof.c b/ext/stackprof/stackprof.c index 6743ba1d..2890ec86 100644 --- a/ext/stackprof/stackprof.c +++ b/ext/stackprof/stackprof.c @@ -366,7 +366,7 @@ stackprof_record_sample_for_stack(int num) _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)) { _stackprof.raw_samples_capa *= 2; _stackprof.raw_samples = realloc(_stackprof.raw_samples, sizeof(VALUE) * _stackprof.raw_samples_capa); } From 0c715715dd88ed5df36f86ef05b1d5eec858ed47 Mon Sep 17 00:00:00 2001 From: Jamie Wong Date: Thu, 16 Nov 2017 17:08:59 -0800 Subject: [PATCH 6/7] Use a separate job handler for gc --- ext/stackprof/stackprof.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/ext/stackprof/stackprof.c b/ext/stackprof/stackprof.c index 2890ec86..81f92128 100644 --- a/ext/stackprof/stackprof.c +++ b/ext/stackprof/stackprof.c @@ -41,6 +41,7 @@ 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; @@ -429,26 +430,40 @@ stackprof_record_sample() } void -stackprof_record_gc_sample() +stackprof_record_gc_samples() { + int i; + _stackprof.frames_buffer[0] = _stackprof.fake_gc_frame; _stackprof.lines_buffer[0] = 0; - stackprof_record_sample_for_stack(1); + 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_job_handler(void *is_gc) +stackprof_gc_job_handler(void *data) { static int in_signal_handler = 0; if (in_signal_handler) return; if (!_stackprof.running) return; in_signal_handler++; - if ((int)is_gc) { - stackprof_record_gc_sample(); - } else { - stackprof_record_sample(); - } + stackprof_record_gc_samples(); + in_signal_handler--; +} + +static void +stackprof_job_handler(void *data) +{ + static int in_signal_handler = 0; + if (in_signal_handler) return; + if (!_stackprof.running) return; + + in_signal_handler++; + stackprof_record_sample(); in_signal_handler--; } @@ -457,8 +472,8 @@ stackprof_signal_handler(int sig, siginfo_t *sinfo, void *ucontext) { _stackprof.overall_signals++; if (rb_during_gc()) { - _stackprof.during_gc++; - rb_postponed_job_register_one(0, stackprof_job_handler, (void*)1); + _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); } From 765380372bfea6c46daaac01ddd908e2ce4aab35 Mon Sep 17 00:00:00 2001 From: Jamie Wong Date: Fri, 17 Nov 2017 12:15:27 -0800 Subject: [PATCH 7/7] Use rb_str_new_cstr instead of rb_str_new_literal --- ext/stackprof/stackprof.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/stackprof/stackprof.c b/ext/stackprof/stackprof.c index 81f92128..9aa2591d 100644 --- a/ext/stackprof/stackprof.c +++ b/ext/stackprof/stackprof.c @@ -579,8 +579,8 @@ Init_stackprof(void) rb_global_variable(&gc_hook); _stackprof.fake_gc_frame = INT2FIX(0x9C); - _stackprof.empty_string = rb_str_new_literal(""); - _stackprof.fake_gc_frame_name = rb_str_new_literal("(garbage collection)"); + _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);