Merge pull request #4320 from DataDog/ivoanjo/prof-9476-improvements-from-managed-string-storage

[PROF-9476] Profiler cleanups extracted from managed string storage branch

Author: ivoanjo

benchmarks/profiler_memory_sample_serialize.rb

@@ -12,7 +12,7 @@
 # This benchmark measures the performance of sampling + serializing memory profiles. It enables us to evaluate changes to
 # the profiler and/or libdatadog that may impact both individual samples, as well as samples over time.
 #
-METRIC_VALUES = { 'cpu-time' => 0, 'cpu-samples' => 0, 'wall-time' => 0, 'alloc-samples' => 1, 'timeline' => 0 }.freeze
+METRIC_VALUES = { 'cpu-time' => 0, 'cpu-samples' => 0, 'wall-time' => 0, 'alloc-samples' => 1, 'timeline' => 0, 'heap_sample' => true }.freeze
 OBJECT_CLASS = 'object'.freeze
 
 def sample_object(recorder, depth = 0)
@@ -56,6 +56,21 @@ def setup
     }
   end
 
+  def create_objects(recorder)
+    samples_per_second = 100
+    simulate_seconds = 60
+    retained_objs = []
+
+    (samples_per_second * simulate_seconds).times do |i|
+      obj = sample_object(recorder, i % 400)
+      retained_objs << obj if (i % @retain_every).zero?
+    end
+
+    GC.start unless @skip_end_gc
+
+    retained_objs
+  end
+
   def run_benchmark
     Benchmark.ips do |x|
       benchmark_time = VALIDATE_BENCHMARK_MODE ? { time: 0.01, warmup: 0 } : { time: 30, warmup: 2 }
@@ -65,18 +80,11 @@ def run_benchmark
 
       x.report("sample+serialize #{ENV['CONFIG']} retain_every=#{@retain_every} heap_samples=#{@heap_samples_enabled} heap_size=#{@heap_size_enabled} heap_sample_every=#{@heap_sample_every} skip_end_gc=#{@skip_end_gc}") do
         recorder = @recorder_factory.call
-        samples_per_second = 100
-        simulate_seconds = 60
-        retained_objs = []
-
-        (samples_per_second * simulate_seconds).times do |i|
-          obj = sample_object(recorder, i % 400)
-          retained_objs << obj if (i % @retain_every).zero?
-        end
-
-        GC.start unless @skip_end_gc
+        retained_objs = create_objects(recorder)
 
         recorder.serialize
+
+        retained_objs.size # Dummy action to make sure this is still alive
       end
 
       x.save! "#{File.basename(__FILE__)}-results.json" unless VALIDATE_BENCHMARK_MODE

ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c

@@ -17,7 +17,7 @@
 #include "setup_signal_handler.h"
 #include "time_helpers.h"
 
-// Used to trigger the execution of Collectors::ThreadState, which implements all of the sampling logic
+// Used to trigger the execution of Collectors::ThreadContext, which implements all of the sampling logic
 // itself; this class only implements the "when to do it" part.
 //
 // This file implements the native bits of the Datadog::Profiling::Collectors::CpuAndWallTimeWorker class
@@ -33,7 +33,7 @@
 // Currently, sampling Ruby threads requires calling Ruby VM APIs that are only safe to call while holding on to the
 // global VM lock (and are not async-signal safe -- cannot be called from a signal handler).
 //
-// @ivoanjo: As a note, I don't think we should think of this constraint as set in stone. Since can reach into the Ruby
+// @ivoanjo: As a note, I don't think we should think of this constraint as set in stone. Since we can reach inside the Ruby
 // internals, we may be able to figure out a way of overcoming it. But it's definitely going to be hard so for now
 // we're considering it as a given.
 //

ext/datadog_profiling_native_extension/collectors_stack.h

@@ -4,8 +4,8 @@
 
 #include "stack_recorder.h"
 
-#define MAX_FRAMES_LIMIT            10000
-#define MAX_FRAMES_LIMIT_AS_STRING "10000"
+#define MAX_FRAMES_LIMIT            3000
+#define MAX_FRAMES_LIMIT_AS_STRING "3000"
 
 typedef struct sampling_buffer sampling_buffer;
 

ext/datadog_profiling_native_extension/collectors_thread_context.c

@@ -1450,11 +1450,8 @@ void thread_context_collector_sample_allocation(VALUE self_instance, unsigned in
 
   // Since this is stack allocated, be careful about moving it
   ddog_CharSlice class_name;
-  ddog_CharSlice *optional_class_name = NULL;
   char imemo_type[100];
 
-  optional_class_name = &class_name;
-
   if (
     type == RUBY_T_OBJECT   ||
     type == RUBY_T_CLASS    ||
@@ -1510,7 +1507,7 @@ void thread_context_collector_sample_allocation(VALUE self_instance, unsigned in
     class_name = ruby_vm_type; // For other weird internal things we just use the VM type
   }
 
-  track_object(state->recorder_instance, new_object, sample_weight, optional_class_name);
+  track_object(state->recorder_instance, new_object, sample_weight, class_name);
 
   per_thread_context *thread_context = get_or_create_context_for(current_thread, state);
 
@@ -1523,7 +1520,7 @@ void thread_context_collector_sample_allocation(VALUE self_instance, unsigned in
     (sample_values) {.alloc_samples = sample_weight, .alloc_samples_unscaled = 1, .heap_sample = true},
     INVALID_TIME, // For now we're not collecting timestamps for allocation events, as per profiling team internal discussions
     &ruby_vm_type,
-    optional_class_name,
+     &class_name,
     /* is_gvl_waiting_state: */ false,
     /* is_safe_to_allocate_objects: */ false // Not safe to allocate further inside the NEWOBJ tracepoint
   );