Merge pull request #4020 from DataDog/ivoanjo/prof-9470-heap-cleanup-…

…after-gc [PROF-9470] Align heap recorder cleanup with GC activity (second try)
DataDog · Oct 24, 2024 · 1a6e255 · 1a6e255
2 parents ee50bd2 + dd653ce
commit 1a6e255
Show file tree

Hide file tree

Showing 23 changed files with 599 additions and 139 deletions.
diff --git a/.gitlab/benchmarks.yml b/.gitlab/benchmarks.yml
@@ -95,22 +95,23 @@ only-profiling-alloc:
     DD_PROFILING_ALLOCATION_ENABLED: "true"
     ADD_TO_GEMFILE: "gem 'datadog', github: 'datadog/dd-trace-rb', ref: '$CI_COMMIT_SHA'"
 
-only-profiling-alloc-ddprof:
+only-profiling-heap:
   extends: .benchmarks
   variables:
-    DD_BENCHMARKS_DDPROF: "true"
     DD_BENCHMARKS_CONFIGURATION: only-profiling
     DD_PROFILING_ENABLED: "true"
     DD_PROFILING_ALLOCATION_ENABLED: "true"
+    DD_PROFILING_EXPERIMENTAL_HEAP_ENABLED: "true"
     ADD_TO_GEMFILE: "gem 'datadog', github: 'datadog/dd-trace-rb', ref: '$CI_COMMIT_SHA'"
 
-only-profiling-heap:
+only-profiling-heap-clean-after-gc:
   extends: .benchmarks
   variables:
     DD_BENCHMARKS_CONFIGURATION: only-profiling
     DD_PROFILING_ENABLED: "true"
     DD_PROFILING_ALLOCATION_ENABLED: "true"
     DD_PROFILING_EXPERIMENTAL_HEAP_ENABLED: "true"
+    DD_PROFILING_HEAP_CLEAN_AFTER_GC_ENABLED: "true"
     ADD_TO_GEMFILE: "gem 'datadog', github: 'datadog/dd-trace-rb', ref: '$CI_COMMIT_SHA'"
 
 profiling-and-tracing:

diff --git a/benchmarks/profiler_gc.rb b/benchmarks/profiler_gc.rb
@@ -9,14 +9,7 @@
 
 class ProfilerGcBenchmark
   def create_profiler
-    @recorder = Datadog::Profiling::StackRecorder.new(
-      cpu_time_enabled: true,
-      alloc_samples_enabled: false,
-      heap_samples_enabled: false,
-      heap_size_enabled: false,
-      heap_sample_every: 1,
-      timeline_enabled: true,
-    )
+    @recorder = Datadog::Profiling::StackRecorder.for_testing(timeline_enabled: true)
     @collector = Datadog::Profiling::Collectors::ThreadContext.for_testing(recorder: @recorder, timeline_enabled: true)
 
     # We take a dummy sample so that the context for the main thread is created, as otherwise the GC profiling methods do

diff --git a/benchmarks/profiler_memory_sample_serialize.rb b/benchmarks/profiler_memory_sample_serialize.rb
@@ -45,7 +45,7 @@ def setup
     @retain_every = (ENV['RETAIN_EVERY'] || '10').to_i
     @skip_end_gc = ENV['SKIP_END_GC'] == 'true'
     @recorder_factory = proc {
-      Datadog::Profiling::StackRecorder.new(
+      Datadog::Profiling::StackRecorder.for_testing(
         cpu_time_enabled: false,
         alloc_samples_enabled: true,
         heap_samples_enabled: @heap_samples_enabled,

diff --git a/benchmarks/profiler_sample_gvl.rb b/benchmarks/profiler_sample_gvl.rb
@@ -31,14 +31,7 @@ def initialize
   end
 
   def create_profiler
-    @recorder = Datadog::Profiling::StackRecorder.new(
-      cpu_time_enabled: true,
-      alloc_samples_enabled: false,
-      heap_samples_enabled: false,
-      heap_size_enabled: false,
-      heap_sample_every: 1,
-      timeline_enabled: true,
-    )
+    @recorder = Datadog::Profiling::StackRecorder.for_testing(timeline_enabled: true)
     @collector = Datadog::Profiling::Collectors::ThreadContext.for_testing(
       recorder: @recorder,
       waiting_for_gvl_threshold_ns: 0,

diff --git a/benchmarks/profiler_sample_loop_v2.rb b/benchmarks/profiler_sample_loop_v2.rb
@@ -13,14 +13,7 @@ class ProfilerSampleLoopBenchmark
   PROFILER_OVERHEAD_STACK_THREAD = Thread.new { sleep }
 
   def create_profiler
-    @recorder = Datadog::Profiling::StackRecorder.new(
-      cpu_time_enabled: true,
-      alloc_samples_enabled: false,
-      heap_samples_enabled: false,
-      heap_size_enabled: false,
-      heap_sample_every: 1,
-      timeline_enabled: false,
-    )
+    @recorder = Datadog::Profiling::StackRecorder.for_testing
     @collector = Datadog::Profiling::Collectors::ThreadContext.for_testing(recorder: @recorder)
   end
 

diff --git a/benchmarks/profiler_sample_serialize.rb b/benchmarks/profiler_sample_serialize.rb
@@ -19,14 +19,7 @@ class ProfilerSampleSerializeBenchmark
 
   def create_profiler
     timeline_enabled = ENV['TIMELINE'] == 'true'
-    @recorder = Datadog::Profiling::StackRecorder.new(
-      cpu_time_enabled: true,
-      alloc_samples_enabled: false,
-      heap_samples_enabled: false,
-      heap_size_enabled: false,
-      heap_sample_every: 1,
-      timeline_enabled: timeline_enabled,
-    )
+    @recorder = Datadog::Profiling::StackRecorder.for_testing(timeline_enabled: timeline_enabled)
     @collector = Datadog::Profiling::Collectors::ThreadContext.for_testing(recorder: @recorder, timeline_enabled: timeline_enabled)
   end
 

diff --git a/ext/datadog_profiling_native_extension/collectors_thread_context.c b/ext/datadog_profiling_native_extension/collectors_thread_context.c
@@ -815,6 +815,9 @@ VALUE thread_context_collector_sample_after_gc(VALUE self_instance) {
 
   state->stats.gc_samples++;
 
+  // Let recorder do any cleanup/updates it requires after a GC step.
+  recorder_after_gc_step(state->recorder_instance);
+
   // Return a VALUE to make it easier to call this function from Ruby APIs that expect a return value (such as rb_rescue2)
   return Qnil;
 }
@@ -1441,7 +1444,8 @@ void thread_context_collector_sample_allocation(VALUE self_instance, unsigned in
         class_name = ruby_value_type_to_class_name(type);
       }
     } else {
-      // Fallback for objects with no class
+      // Fallback for objects with no class. Objects with no class are a way for the Ruby VM to mark them
+      // as internal objects; see rb_objspace_internal_object_p for details.
       class_name = ruby_value_type_to_class_name(type);
     }
   } else if (type == RUBY_T_IMEMO) {