From e069a4a78f75ce847486459a0d9be7ed0af81361 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Fri, 13 Sep 2024 14:08:01 +0100 Subject: [PATCH] Add testing for enabling the GVL profiling hook + a small refactor for skipping GVL profiling specs on legacy Rubies --- .../collectors_cpu_and_wall_time_worker.c | 13 +++++ .../cpu_and_wall_time_worker_spec.rb | 52 +++++++++++++++++-- .../collectors/thread_context_spec.rb | 14 ++--- spec/datadog/profiling/spec_helper.rb | 4 +- 4 files changed, 69 insertions(+), 14 deletions(-) diff --git a/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c b/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c index d2de40f905e..9296fb7e753 100644 --- a/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c +++ b/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c @@ -230,6 +230,7 @@ static VALUE _native_resume_signals(DDTRACE_UNUSED VALUE self); static void on_gvl_event(rb_event_flag_t event_id, const rb_internal_thread_event_data_t *event_data, DDTRACE_UNUSED void *_unused); #endif static void after_gvl_running_from_postponed_job(DDTRACE_UNUSED void *_unused); +static VALUE _native_gvl_profiling_hook_active(DDTRACE_UNUSED VALUE self, VALUE instance); // We're using `on_newobj_event` function with `rb_add_event_hook2`, which requires in its public signature a function // with signature `rb_event_hook_func_t` which doesn't match `on_newobj_event`. @@ -325,6 +326,7 @@ void collectors_cpu_and_wall_time_worker_init(VALUE profiling_module) { rb_define_singleton_method(testing_module, "_native_is_sigprof_blocked_in_current_thread", _native_is_sigprof_blocked_in_current_thread, 0); rb_define_singleton_method(testing_module, "_native_with_blocked_sigprof", _native_with_blocked_sigprof, 0); rb_define_singleton_method(testing_module, "_native_delayed_error", _native_delayed_error, 2); + rb_define_singleton_method(testing_module, "_native_gvl_profiling_hook_active", _native_gvl_profiling_hook_active, 1); } // This structure is used to define a Ruby object that stores a pointer to a struct cpu_and_wall_time_worker_state @@ -1338,3 +1340,14 @@ static VALUE _native_resume_signals(DDTRACE_UNUSED VALUE self) { state->during_sample = false; } #endif + +static VALUE _native_gvl_profiling_hook_active(DDTRACE_UNUSED VALUE self, VALUE instance) { + #ifndef NO_GVL_INSTRUMENTATION + struct cpu_and_wall_time_worker_state *state; + TypedData_Get_Struct(instance, struct cpu_and_wall_time_worker_state, &cpu_and_wall_time_worker_typed_data, state); + + return state->gvl_profiling_hook != NULL ? Qtrue : Qfalse; + #else + return Qfalse; + #endif +} diff --git a/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb b/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb index c82dd82d570..4f2c073d655 100644 --- a/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb +++ b/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb @@ -136,6 +136,28 @@ end end + context "when gvl_profiling_enabled is true" do + before { skip_if_gvl_profiling_not_supported(self) } + + let(:gvl_profiling_enabled) { true } + + it "enables the gvl profiling hook" do + start + + expect(described_class::Testing._native_gvl_profiling_hook_active(cpu_and_wall_time_worker)).to be true + end + end + + context "when gvl_profiling_enabled is false" do + let(:gvl_profiling_enabled) { false } + + it "does not enable the gvl profiling hook" do + start + + expect(described_class::Testing._native_gvl_profiling_hook_active(cpu_and_wall_time_worker)).to be false + end + end + context "when a previous signal handler existed" do before do described_class::Testing._native_install_testing_signal_handler @@ -374,11 +396,7 @@ end context "when GVL profiling is enabled" do - before do - if min_ruby_for_gvl_profiling > RUBY_VERSION - skip "GVL profiling is is only supported on Ruby >= #{min_ruby_for_gvl_profiling}" - end - end + before { skip_if_gvl_profiling_not_supported(self) } let(:gvl_profiling_enabled) { true } @@ -947,6 +965,18 @@ # come from us -- it's the default message for an unhandled SIGPROF. Pretty confusing UNIX/POSIX behavior...) Process.kill("SIGPROF", Process.pid) end + + context "when GVL profiling is enabled" do + before { skip_if_gvl_profiling_not_supported(self) } + + let(:gvl_profiling_enabled) { true } + + it "disables the GVL profiling hook" do + expect { stop } + .to change { described_class::Testing._native_gvl_profiling_hook_active(cpu_and_wall_time_worker) } + .from(true).to(false) + end + end end it "unblocks SIGPROF signal handling from the worker thread" do @@ -993,6 +1023,18 @@ .from(true).to(false) end + context "when gvl_profiling_enabled is true" do + before { skip_if_gvl_profiling_not_supported(self) } + + let(:gvl_profiling_enabled) { true } + + it "disables the gvl profiling hook" do + expect { reset_after_fork } + .to change { described_class::Testing._native_gvl_profiling_hook_active(cpu_and_wall_time_worker) } + .from(true).to(false) + end + end + it "resets the CpuAndWallTime collector only after disabling the tracepoint" do expect(thread_context_collector).to receive(:reset_after_fork) do expect(described_class::Testing._native_gc_tracepoint(cpu_and_wall_time_worker)).to_not be_enabled diff --git a/spec/datadog/profiling/collectors/thread_context_spec.rb b/spec/datadog/profiling/collectors/thread_context_spec.rb index dc0447fe7b6..9b81616500c 100644 --- a/spec/datadog/profiling/collectors/thread_context_spec.rb +++ b/spec/datadog/profiling/collectors/thread_context_spec.rb @@ -780,7 +780,7 @@ def self.otel_sdk_available? context "when thread starts Waiting for GVL" do before do - skip "Behavior does not apply to current Ruby version" if min_ruby_for_gvl_profiling > RUBY_VERSION + skip_if_gvl_profiling_not_supported(self) sample # trigger context creation samples_from_pprof(recorder.serialize!) # flush sample @@ -843,7 +843,7 @@ def self.otel_sdk_available? context "when thread is Waiting for GVL" do before do - skip "Behavior does not apply to current Ruby version" if min_ruby_for_gvl_profiling > RUBY_VERSION + skip_if_gvl_profiling_not_supported(self) sample # trigger context creation on_gvl_waiting(t1) @@ -1465,7 +1465,7 @@ def sample_and_check(expected_state:) end describe "#on_gvl_waiting" do - before { skip "Behavior does not apply to current Ruby version" if min_ruby_for_gvl_profiling > RUBY_VERSION } + before { skip_if_gvl_profiling_not_supported(self) } context "if thread has not been sampled before" do it "does not record anything in the internal_thread_specific value" do @@ -1491,7 +1491,7 @@ def sample_and_check(expected_state:) end describe "#on_gvl_running" do - before { skip "Behavior does not apply to current Ruby version" if min_ruby_for_gvl_profiling > RUBY_VERSION } + before { skip_if_gvl_profiling_not_supported(self) } context "if thread has not been sampled before" do it "does not record anything in the internal_thread_specific value" do @@ -1571,7 +1571,7 @@ def sample_and_check(expected_state:) end describe "#sample_after_gvl_running" do - before { skip "Behavior does not apply to current Ruby version" if min_ruby_for_gvl_profiling > RUBY_VERSION } + before { skip_if_gvl_profiling_not_supported(self) } let(:timeline_enabled) { true } @@ -1823,7 +1823,7 @@ def sample_and_check(expected_state:) describe ":gvl_waiting_at" do context "on supported Rubies" do - before { skip "Behavior does not apply to current Ruby version" if min_ruby_for_gvl_profiling > RUBY_VERSION } + before { skip_if_gvl_profiling_not_supported(self) } it "is initialized to GVL_WAITING_ENABLED_EMPTY (INTPTR_MAX)" do expect(per_thread_context.values).to all( @@ -1833,7 +1833,7 @@ def sample_and_check(expected_state:) end context "on legacy Rubies" do - before { skip "Behavior does not apply to current Ruby version" if min_ruby_for_gvl_profiling <= RUBY_VERSION } + before { skip "Behavior does not apply to current Ruby version" if RUBY_VERSION >= "3.3." } it "is not set" do per_thread_context.each do |_thread, context| diff --git a/spec/datadog/profiling/spec_helper.rb b/spec/datadog/profiling/spec_helper.rb index 92d3fa36ae5..205000b7f32 100644 --- a/spec/datadog/profiling/spec_helper.rb +++ b/spec/datadog/profiling/spec_helper.rb @@ -122,8 +122,8 @@ def self.maybe_fix_label_range(key, value) end end - def min_ruby_for_gvl_profiling - "3.3." + def skip_if_gvl_profiling_not_supported(testcase) + testcase.skip "GVL profiling is only supported on Ruby >= 3.3" if RUBY_VERSION < "3.3." end end