Skip to content

Commit

Permalink
[PROF-8667] Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
AlexJF committed Jan 5, 2024
1 parent 4e29094 commit 63c286e
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 49 deletions.
36 changes: 18 additions & 18 deletions ext/ddtrace_profiling_native_extension/heap_recorder.c
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ static object_record* object_record_new(long, heap_record*, live_object_data);
static void object_record_free(object_record*);

struct heap_recorder {
// Config
// Whether the recorder should try to determine approximate sizes for tracked objects.
bool size_enabled;

// Map[key: heap_record_key*, record: heap_record*]
// NOTE: We always use heap_record_key.type == HEAP_STACK for storage but support lookups
// via heap_record_key.type == LOCATION_SLICE to allow for allocation-free fast-paths.
Expand Down Expand Up @@ -149,6 +153,7 @@ heap_recorder* heap_recorder_new(void) {
recorder->object_records_snapshot = NULL;
recorder->reusable_locations = ruby_xcalloc(MAX_FRAMES_LIMIT, sizeof(ddog_prof_Location));
recorder->partial_object_record = NULL;
recorder->size_enabled = true;

return recorder;
}
Expand Down Expand Up @@ -182,6 +187,10 @@ void heap_recorder_free(heap_recorder *heap_recorder) {
ruby_xfree(heap_recorder);
}

void heap_recorder_set_size_enabled(heap_recorder *heap_recorder, bool size_enabled) {
heap_recorder->size_enabled = size_enabled;
}

// WARN: Assumes this gets called before profiler is reinitialized on the fork
void heap_recorder_after_fork(heap_recorder *heap_recorder) {
if (heap_recorder == NULL) {
Expand Down Expand Up @@ -250,15 +259,7 @@ void end_heap_allocation_recording(struct heap_recorder *heap_recorder, ddog_pro
commit_allocation(heap_recorder, heap_record, partial_object_record);
}

typedef struct {
// A reference to the heap recorder so we can access extra stuff to cleanup unused records.
heap_recorder *heap_recorder;

// Whether we should update object sizes as part of the update iteration or not.
bool update_sizes;
} prepare_iteration_context;

void heap_recorder_prepare_iteration(heap_recorder *heap_recorder, bool update_sizes) {
void heap_recorder_prepare_iteration(heap_recorder *heap_recorder) {
if (heap_recorder == NULL) {
return;
}
Expand All @@ -268,11 +269,7 @@ void heap_recorder_prepare_iteration(heap_recorder *heap_recorder, bool update_s
rb_raise(rb_eRuntimeError, "New heap recorder iteration prepared without the previous one having been finished.");
}

prepare_iteration_context context = (prepare_iteration_context) {
.heap_recorder = heap_recorder,
.update_sizes = update_sizes,
};
st_foreach(heap_recorder->object_records, st_object_record_update, (st_data_t) &context);
st_foreach(heap_recorder->object_records, st_object_record_update, (st_data_t) heap_recorder);

heap_recorder->object_records_snapshot = st_copy(heap_recorder->object_records);
if (heap_recorder->object_records_snapshot == NULL) {
Expand Down Expand Up @@ -376,7 +373,7 @@ static int st_object_record_entry_free(DDTRACE_UNUSED st_data_t key, st_data_t v
static int st_object_record_update(st_data_t key, st_data_t value, st_data_t extra_arg) {
long obj_id = (long) key;
object_record *record = (object_record*) value;
prepare_iteration_context *context = (prepare_iteration_context*) extra_arg;
heap_recorder *recorder = (heap_recorder*) extra_arg;

VALUE ref;

Expand All @@ -388,17 +385,20 @@ static int st_object_record_update(st_data_t key, st_data_t value, st_data_t ext
heap_record->num_tracked_objects--;

// One less object using this heap record, it may have become unused...
cleanup_heap_record_if_unused(context->heap_recorder, heap_record);
cleanup_heap_record_if_unused(recorder, heap_record);

object_record_free(record);
return ST_DELETE;
}

// If we got this far, entry is still valid

if (context->update_sizes) {
// if we were asked to update sizes, do so...
if (recorder->size_enabled && !record->object_data.is_frozen) {
// if we were asked to update sizes and this object was not already seen as being frozen,
// update size again.
record->object_data.size = ruby_obj_memsize_of(ref);
// Check if it's now frozen so we skip a size update next time
record->object_data.is_frozen = RB_OBJ_FROZEN(ref);
}

return ST_CONTINUE;
Expand Down
22 changes: 17 additions & 5 deletions ext/ddtrace_profiling_native_extension/heap_recorder.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ typedef struct live_object_data {
//
// This enables us to calculate the age of this object in terms of GC executions.
size_t alloc_gen;

// Whether this object was previously seen as being frozen. If this is the case,
// we'll skip any further size updates since frozen objects are supposed to be
// immutable.
bool is_frozen;
} live_object_data;

// Data that is made available to iterators of heap recorder data for each live object
Expand All @@ -53,6 +58,17 @@ heap_recorder* heap_recorder_new(void);
// Free a previously initialized heap recorder.
void heap_recorder_free(heap_recorder *heap_recorder);

// Sets whether this heap recorder should keep track of sizes or not.
//
// If set to true, the heap recorder will attempt to determine the approximate sizes of
// tracked objects and wield them during iteration.
// If set to false, sizes returned during iteration should not be used/relied on (they
// may be 0 or the last determined size before disabling the tracking of sizes).
//
// NOTE: Default is true, i.e., it will attempt to determine approximate sizes of tracked
// objects.
void heap_recorder_set_size_enabled(heap_recorder *heap_recorder, bool size_enabled);

// Do any cleanup needed after forking.
// WARN: Assumes this gets called before profiler is reinitialized on the fork
void heap_recorder_after_fork(heap_recorder *heap_recorder);
Expand Down Expand Up @@ -82,12 +98,8 @@ void end_heap_allocation_recording(heap_recorder *heap_recorder, ddog_prof_Slice
// Update the heap recorder to reflect the latest state of the VM and prepare internal structures
// for efficient iteration.
//
// @param update_sizes
// True if we should re-calculate live object sizes ahead of the next iteration. If false,
// the previous sizes will be used (or 0 if we never updated them before).
//
// WARN: This must be called strictly before iteration. Failing to do so will result in exceptions.
void heap_recorder_prepare_iteration(heap_recorder *heap_recorder, bool update_sizes);
void heap_recorder_prepare_iteration(heap_recorder *heap_recorder);

// Optimize the heap recorder by cleaning up any data that might have been prepared specifically
// for the purpose of iterating over the heap recorder data.
Expand Down
10 changes: 9 additions & 1 deletion ext/ddtrace_profiling_native_extension/ruby_helpers.c
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,14 @@ bool ruby_ref_from_id(VALUE obj_id, VALUE *value) {
size_t rb_obj_memsize_of(VALUE obj);

// Wrapper around rb_obj_memsize_of to avoid hitting crashing paths.
//
// The crashing paths are due to calls to rb_bug so should hopefully
// be situations that can't happen. But given that rb_obj_memsize_of
// isn't fully public (it's externed but not part of public headers)
// there is a possibility that it is just assumed that whoever calls
// it, will do proper checking for those cases. We want to be cautious
// so we'll assume that's the case and will skip over known crashing
// paths in this wrapper.
size_t ruby_obj_memsize_of(VALUE obj) {
switch (rb_type(obj)) {
case T_OBJECT:
Expand All @@ -190,7 +198,7 @@ size_t ruby_obj_memsize_of(VALUE obj) {
case T_FLOAT:
case T_SYMBOL:
case T_BIGNUM:
// case T_NODE: -> Throws exception in rb_obj_memsize_of
// case T_NODE: -> Crashes the vm in rb_obj_memsize_of
case T_STRUCT:
case T_ZOMBIE:
#ifndef NO_T_MOVED
Expand Down
11 changes: 4 additions & 7 deletions ext/ddtrace_profiling_native_extension/stack_recorder.c
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,6 @@ struct stack_recorder_state {

uint8_t position_for[ALL_VALUE_TYPES_COUNT];
uint8_t enabled_values_count;
bool heap_size_enabled;
};

// Used to return a pair of values from sampler_lock_active_profile()
Expand Down Expand Up @@ -217,7 +216,7 @@ static VALUE _native_initialize(
VALUE cpu_time_enabled,
VALUE alloc_samples_enabled,
VALUE heap_samples_enabled,
VALUE heap_sizes_enabled,
VALUE heap_size_enabled,
VALUE timeline_enabled
);
static VALUE _native_serialize(VALUE self, VALUE recorder_instance);
Expand Down Expand Up @@ -311,7 +310,6 @@ static VALUE _native_new(VALUE klass) {
// heap samples, we will free and reset heap_recorder to NULL, effectively disabling all behaviour specific
// to heap profiling (all calls to heap_recorder_* with a NULL heap recorder are noops).
state->heap_recorder = heap_recorder_new();
state->heap_size_enabled = true;

// Note: Don't raise exceptions after this point, since it'll lead to libdatadog memory leaking!

Expand Down Expand Up @@ -435,11 +433,10 @@ static VALUE _native_initialize(
if (heap_size_enabled == Qtrue) {
enabled_value_types[next_enabled_pos] = (ddog_prof_ValueType) HEAP_SIZE_VALUE;
state->position_for[HEAP_SIZE_VALUE_ID] = next_enabled_pos++;
state->heap_size_enabled = true;
} else {
state->position_for[HEAP_SIZE_VALUE_ID] = next_disabled_pos++;
state->heap_size_enabled = false;
}
heap_recorder_set_size_enabled(state->heap_recorder, heap_size_enabled);

if (heap_samples_enabled == Qfalse && heap_size_enabled == Qfalse) {
// Turns out heap sampling is disabled but we initialized everything in _native_new
Expand Down Expand Up @@ -474,7 +471,7 @@ static VALUE _native_serialize(DDTRACE_UNUSED VALUE _self, VALUE recorder_instan

// Prepare the iteration on heap recorder we'll be doing outside the GVL. The preparation needs to
// happen while holding on to the GVL.
heap_recorder_prepare_iteration(state->heap_recorder, state->heap_size_enabled);
heap_recorder_prepare_iteration(state->heap_recorder);

// We'll release the Global VM Lock while we're calling serialize, so that the Ruby VM can continue to work while this
// is pending
Expand Down Expand Up @@ -882,7 +879,7 @@ static VALUE _native_start_fake_slow_heap_serialization(DDTRACE_UNUSED VALUE _se
struct stack_recorder_state *state;
TypedData_Get_Struct(recorder_instance, struct stack_recorder_state, &stack_recorder_typed_data, state);

heap_recorder_prepare_iteration(state->heap_recorder, false);
heap_recorder_prepare_iteration(state->heap_recorder);

return Qnil;
}
Expand Down
21 changes: 3 additions & 18 deletions spec/datadog/profiling/stack_recorder_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -449,28 +449,13 @@ def sample_allocation(obj)

it 'include accurate object sizes' do
string_sample = heap_samples.find { |s| s.labels[:'allocation class'] == 'String' }
expect(string_sample.values[:'heap-live-size']).to be_between(
# 18 UTF-8 characters at minimum.
(18 * 2) * sample_rate,
# Add some extra padding (per char and object-wide) for extra data.
(18 * 4 + 40) * sample_rate
)
expect(string_sample.values[:'heap-live-size']).to eq(ObjectSpace.memsize_of(a_string) * sample_rate)

array_sample = heap_samples.find { |s| s.labels[:'allocation class'] == 'Array' }
expect(array_sample.values[:'heap-live-size']).to be_between(
# Basic object + 100 FIXNUMs (32 bits)
(40 + 100 * 4) * sample_rate,
# Basic object + 128 FIXNUMs (64 bits and round to nearest power of 2) and eventual extra data
(40 + 128 * 8 + 10) * sample_rate,
)
expect(array_sample.values[:'heap-live-size']).to eq(ObjectSpace.memsize_of(an_array) * sample_rate)

hash_sample = heap_samples.find { |s| s.labels[:'allocation class'] == 'Hash' }
expect(hash_sample.values[:'heap-live-size']).to be_between(
# Basic object + 4 table entries + no bins
(40 + 4 * 16) * sample_rate,
# Add extra padding to hash itself as well as each entry and 8 bins
(80 + 4 * 32 + 8 * 16) * sample_rate,
)
expect(hash_sample.values[:'heap-live-size']).to eq(ObjectSpace.memsize_of(a_hash) * sample_rate)
end

it 'include accurate object ages' do
Expand Down

0 comments on commit 63c286e

Please sign in to comment.