[PROF-8543] Address comments

benchmarks/profiler_sample_loop_v2.rb

@@ -18,8 +18,8 @@ class ProfilerSampleLoopBenchmark
   def create_profiler
     @recorder = Datadog::Profiling::StackRecorder.new(
       cpu_time_enabled: true,
-      alloc_samples_enabled: true,
-      heap_samples_enabled: true
+      alloc_samples_enabled: false,
+      heap_samples_enabled: false
     )
     @collector = Datadog::Profiling::Collectors::ThreadContext.new(
       recorder: @recorder, max_frames: 400, tracer: nil, endpoint_collection_enabled: false, timeline_enabled: false

ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time_worker.c

@@ -81,10 +81,11 @@ struct cpu_and_wall_time_worker_state {
 
   bool gc_profiling_enabled;
   bool allocation_counting_enabled;
-  bool heap_counting_enabled;
   bool no_signals_workaround_enabled;
   bool dynamic_sampling_rate_enabled;
-  int allocation_sample_every; // Temporarily used for development/testing of allocation profiling
+  int allocation_sample_every;
+  bool allocation_profiling_enabled;
+  bool heap_profiling_enabled;
   VALUE self_instance;
   VALUE thread_context_collector_instance;
   VALUE idle_sampling_helper_instance;
@@ -152,10 +153,11 @@ static VALUE _native_initialize(
   VALUE gc_profiling_enabled,
   VALUE idle_sampling_helper_instance,
   VALUE allocation_counting_enabled,
-  VALUE heap_counting_enabled,
   VALUE no_signals_workaround_enabled,
   VALUE dynamic_sampling_rate_enabled,
-  VALUE allocation_sample_every
+  VALUE allocation_sample_every,
+  VALUE allocation_profiling_enabled,
+  VALUE heap_profiling_enabled
 );
 static void cpu_and_wall_time_worker_typed_data_mark(void *state_ptr);
 static VALUE _native_sampling_loop(VALUE self, VALUE instance);
@@ -193,7 +195,6 @@ static void on_freeobj_event(VALUE tracepoint_data, DDTRACE_UNUSED void *unused)
 static void disable_tracepoints(struct cpu_and_wall_time_worker_state *state);
 static VALUE _native_with_blocked_sigprof(DDTRACE_UNUSED VALUE self);
 static VALUE rescued_sample_allocation(VALUE tracepoint_data);
-static VALUE rescued_sample_free(VALUE tracepoint_data);
 
 // Note on sampler global state safety:
 //
@@ -231,7 +232,7 @@ void collectors_cpu_and_wall_time_worker_init(VALUE profiling_module) {
   // https://bugs.ruby-lang.org/issues/18007 for a discussion around this.
   rb_define_alloc_func(collectors_cpu_and_wall_time_worker_class, _native_new);
 
-  rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_initialize", _native_initialize, 9);
+  rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_initialize", _native_initialize, 10);
   rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_sampling_loop", _native_sampling_loop, 1);
   rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_stop", _native_stop, 2);
   rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_reset_after_fork", _native_reset_after_fork, 1);
@@ -270,10 +271,11 @@ static VALUE _native_new(VALUE klass) {
 
   state->gc_profiling_enabled = false;
   state->allocation_counting_enabled = false;
-  state->heap_counting_enabled = false;
   state->no_signals_workaround_enabled = false;
   state->dynamic_sampling_rate_enabled = true;
   state->allocation_sample_every = 0;
+  state->allocation_profiling_enabled = false;
+  state->heap_profiling_enabled = false;
   state->thread_context_collector_instance = Qnil;
   state->idle_sampling_helper_instance = Qnil;
   state->owner_thread = Qnil;
@@ -300,30 +302,37 @@ static VALUE _native_initialize(
   VALUE gc_profiling_enabled,
   VALUE idle_sampling_helper_instance,
   VALUE allocation_counting_enabled,
-  VALUE heap_counting_enabled,
   VALUE no_signals_workaround_enabled,
   VALUE dynamic_sampling_rate_enabled,
-  VALUE allocation_sample_every
+  VALUE allocation_sample_every,
+  VALUE allocation_profiling_enabled,
+  VALUE heap_profiling_enabled
 ) {
   ENFORCE_BOOLEAN(gc_profiling_enabled);
   ENFORCE_BOOLEAN(allocation_counting_enabled);
-  ENFORCE_BOOLEAN(heap_counting_enabled);
   ENFORCE_BOOLEAN(no_signals_workaround_enabled);
   ENFORCE_BOOLEAN(dynamic_sampling_rate_enabled);
   ENFORCE_TYPE(allocation_sample_every, T_FIXNUM);
+  ENFORCE_BOOLEAN(allocation_profiling_enabled);
+  ENFORCE_BOOLEAN(heap_profiling_enabled);
 
   struct cpu_and_wall_time_worker_state *state;
   TypedData_Get_Struct(self_instance, struct cpu_and_wall_time_worker_state, &cpu_and_wall_time_worker_typed_data, state);
 
   state->gc_profiling_enabled = (gc_profiling_enabled == Qtrue);
   state->allocation_counting_enabled = (allocation_counting_enabled == Qtrue);
-  state->heap_counting_enabled = state->allocation_counting_enabled && (heap_counting_enabled == Qtrue);
   state->no_signals_workaround_enabled = (no_signals_workaround_enabled == Qtrue);
   state->dynamic_sampling_rate_enabled = (dynamic_sampling_rate_enabled == Qtrue);
   state->allocation_sample_every = NUM2INT(allocation_sample_every);
+  state->allocation_profiling_enabled = (allocation_profiling_enabled == Qtrue);
+  state->heap_profiling_enabled = (heap_profiling_enabled == Qtrue);
 
-  if (state->allocation_sample_every < 0) {
-    rb_raise(rb_eArgError, "Unexpected value for allocation_sample_every: %d. This value must be >= 0.", state->allocation_sample_every);
+  if (state->allocation_sample_every <= 0) {
+    rb_raise(rb_eArgError, "Unexpected value for allocation_sample_every: %d. This value must be > 0.", state->allocation_sample_every);
+  }
+
+  if (state->heap_profiling_enabled && !state->allocation_profiling_enabled) {
+    rb_raise(rb_eArgError, "Heap profiling requires allocation profiling to be enabled but it isn't.");
   }
 
   state->thread_context_collector_instance = enforce_thread_context_collector_instance(thread_context_collector_instance);
@@ -644,8 +653,8 @@ static VALUE release_gvl_and_run_sampling_trigger_loop(VALUE instance) {
   // because they may raise exceptions.
   install_sigprof_signal_handler(handle_sampling_signal, "handle_sampling_signal");
   if (state->gc_profiling_enabled) rb_tracepoint_enable(state->gc_tracepoint);
-  if (state->allocation_counting_enabled) rb_tracepoint_enable(state->object_allocation_tracepoint);
-  if (state->heap_counting_enabled) rb_tracepoint_enable(state->object_free_tracepoint);
+  if (state->allocation_counting_enabled || state->allocation_profiling_enabled) rb_tracepoint_enable(state->object_allocation_tracepoint);
+  if (state->heap_profiling_enabled) rb_tracepoint_enable(state->object_free_tracepoint);
 
   rb_thread_call_without_gvl(run_sampling_trigger_loop, state, interrupt_sampling_trigger_loop, state);
 
@@ -929,38 +938,36 @@ static void on_newobj_event(VALUE tracepoint_data, DDTRACE_UNUSED void *unused)
     return;
   }
 
-  // @ivoanjo: Strictly speaking, this is not needed because Ruby should not call the same tracepoint while a previous
-  // invocation is still pending, (e.g. it wouldn't call `on_newobj_event` while it's already running), but I decided
-  // to keep this here for consistency -- every call to the thread context (other than the special gc calls which are
-  // defined as not being able to allocate) sets this.
   state->during_sample = true;
 
   // TODO: This is a placeholder sampling decision strategy. We plan to replace it with a better one soon (e.g. before
   // beta), and having something here allows us to test the rest of feature, sampling decision aside.
-  if (state->allocation_sample_every > 0 && ((allocation_count % state->allocation_sample_every) == 0)) {
+  if (state->allocation_profiling_enabled && state->allocation_sample_every > 0 && ((allocation_count % state->allocation_sample_every) == 0)) {
     // Rescue against any exceptions that happen during sampling
     safely_call(rescued_sample_allocation, tracepoint_data, state->self_instance);
   }
 
   state->during_sample = false;
 }
 
+// Safety: This function may get called while Ruby is doing garbage collection. While Ruby is doing garbage collection,
+// *NO ALLOCATION* is allowed. This function, and any it calls must never trigger memory or object allocation.
+// This includes exceptions and use of ruby_xcalloc (because xcalloc can trigger GC)!
 static void on_freeobj_event(VALUE tracepoint_data, DDTRACE_UNUSED void *unused) {
   struct cpu_and_wall_time_worker_state *state = active_sampler_instance_state; // Read from global variable, see "sampler global state safety" note above
 
   // This should not happen in a normal situation because the tracepoint is always enabled after the instance is set
   // and disabled before it is cleared, but just in case...
   if (state == NULL) return;
 
-  // @ivoanjo: Strictly speaking, this is not needed because Ruby should not call the same tracepoint while a previous
-  // invocation is still pending, (e.g. it wouldn't call `on_newobj_event` while it's already running), but I decided
-  // to keep this here for consistency -- every call to the thread context (other than the special gc calls which are
-  // defined as not being able to allocate) sets this.
-  state->during_sample = true;
+  // NOTE: Because this is likely to be happening during GC, handling of this tracepoint does not do any allocation.
+  // We also do not want to lose any frees as that would affect the accuracy of our live heap tracking so we skip
+  // the typical `state->during_sample` dropping that other sampling tracepoints have.
 
-  safely_call(rescued_sample_free, tracepoint_data, state->self_instance);
+  rb_trace_arg_t *data = rb_tracearg_from_tracepoint(tracepoint_data);
+  VALUE freed_object = rb_tracearg_object(data);
 
-  state->during_sample = false;
+  thread_context_collector_sample_free(state->thread_context_collector_instance, freed_object);
 }
 
 static void disable_tracepoints(struct cpu_and_wall_time_worker_state *state) {
@@ -996,18 +1003,3 @@ static VALUE rescued_sample_allocation(VALUE tracepoint_data) {
   // Return a dummy VALUE because we're called from rb_rescue2 which requires it
   return Qnil;
 }
-
-static VALUE rescued_sample_free(VALUE tracepoint_data) {
-  struct cpu_and_wall_time_worker_state *state = active_sampler_instance_state; // Read from global variable, see "sampler global state safety" note above
-
-  // This should not happen in a normal situation because on_newobj_event already checked for this, but just in case...
-  if (state == NULL) return Qnil;
-
-  rb_trace_arg_t *data = rb_tracearg_from_tracepoint(tracepoint_data);
-  VALUE freed_object = rb_tracearg_object(data);
-
-  thread_context_collector_sample_free(state->thread_context_collector_instance, freed_object);
-
-  // Return a dummy VALUE because we're called from rb_rescue2 which requires it
-  return Qnil;
-}

ext/ddtrace_profiling_native_extension/collectors_thread_context.c

@@ -1203,7 +1203,7 @@ void thread_context_collector_sample_allocation(VALUE self_instance, unsigned in
     }
   }
 
-  record_obj_allocation(state->recorder_instance, new_object, sample_weight, optional_class_name);
+  track_obj_allocation(state->recorder_instance, new_object, sample_weight);
 
   trigger_sample_for_thread(
     state,
@@ -1218,6 +1218,9 @@ void thread_context_collector_sample_allocation(VALUE self_instance, unsigned in
   );
 }
 
+// Safety: This function may get called while Ruby is doing garbage collection. While Ruby is doing garbage collection,
+// *NO ALLOCATION* is allowed. This function, and any it calls must never trigger memory or object allocation.
+// This includes exceptions and use of ruby_xcalloc (because xcalloc can trigger GC)!
 void thread_context_collector_sample_free(VALUE self_instance, VALUE freed_object) {
   struct thread_context_collector_state *state;
   TypedData_Get_Struct(self_instance, struct thread_context_collector_state, &thread_context_collector_typed_data, state);

ext/ddtrace_profiling_native_extension/extconf.rb

@@ -120,7 +120,7 @@ def add_compiler_flag(flag)
 add_compiler_flag '-Wall'
 add_compiler_flag '-Wextra'
 
-if ENV['DEBUG']
+if ENV['DDTRACE_DEBUG']
   CONFIG['optflags'] = '-O0'
   CONFIG['debugflags'] = '-ggdb3'
 end