Skip to content

Commit

Permalink
[NO-TICKET] Profiling refactor: typedef most structures
Browse files Browse the repository at this point in the history
**What does this PR do?**

In C, when you declare a `struct foo { ... };` (a bag of fields), you
need to always call it `struct foo` by default.

But, using the `typedef` keyword instead, e.g.
`typedef struct { ... } foo;` you can now refer to the type as `foo`.

I've done a big pass and converted almost all structs we have to
a typedef. Some already had both names (`struct foo` and `foo`),
and in those cases I left only the typedef.

**Motivation:**

Historically the profiler codebase has used structs both with and
without the typedef in a quite inconsistent way. I've been meaning
to fix this inconsistency.

**Additional Notes:**

There's a few structs in the heap profiler I did not touch. That's
because I'm working on a PR that has a number of big changes to the
heap profiler and it's not worth doing those changes, introducing
more conflicts, just to then remove many of them as they are unused
and whatnot.

**How to test the change?**

The code successfully compiling + the existing test coverage is
enough to validate these changes.
  • Loading branch information
ivoanjo committed Jan 3, 2025
1 parent b26f306 commit 913fe43
Show file tree
Hide file tree
Showing 15 changed files with 270 additions and 270 deletions.
4 changes: 2 additions & 2 deletions ext/datadog_profiling_native_extension/clock_id.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
#include <ruby.h>

// Contains the operating-system specific identifier needed to fetch CPU-time, and a flag to indicate if we failed to fetch it
typedef struct thread_cpu_time_id {
typedef struct {
bool valid;
clockid_t clock_id;
} thread_cpu_time_id;

// Contains the current cpu time, and a flag to indicate if we failed to fetch it
typedef struct thread_cpu_time {
typedef struct {
bool valid;
long result_ns;
} thread_cpu_time;
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ static VALUE _native_should_sample(VALUE self, VALUE now);
static VALUE _native_after_sample(VALUE self, VALUE now);
static VALUE _native_state_snapshot(VALUE self);

typedef struct sampler_state {
typedef struct {
discrete_dynamic_sampler sampler;
} sampler_state;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
// every event and is thus, in theory, susceptible to some pattern
// biases. In practice, the dynamic readjustment of sampling interval
// and randomized starting point should help with avoiding heavy biases.
typedef struct discrete_dynamic_sampler {
typedef struct {
// --- Config ---
// Name of this sampler for debug logs.
const char *debug_name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@
typedef enum { ACTION_WAIT, ACTION_RUN, ACTION_STOP } action;

// Contains state for a single CpuAndWallTimeWorker instance
struct idle_sampling_loop_state {
typedef struct {
pthread_mutex_t wakeup_mutex;
pthread_cond_t wakeup;
action requested_action;
void (*run_action_function)(void);
};
} idle_sampling_loop_state;

static VALUE _native_new(VALUE klass);
static void reset_state(struct idle_sampling_loop_state *state);
static void reset_state(idle_sampling_loop_state *state);
static VALUE _native_idle_sampling_loop(DDTRACE_UNUSED VALUE self, VALUE self_instance);
static VALUE _native_stop(DDTRACE_UNUSED VALUE self, VALUE self_instance);
static void *run_idle_sampling_loop(void *state_ptr);
Expand Down Expand Up @@ -62,7 +62,7 @@ void collectors_idle_sampling_helper_init(VALUE profiling_module) {
rb_define_singleton_method(testing_module, "_native_idle_sampling_helper_request_action", _native_idle_sampling_helper_request_action, 1);
}

// This structure is used to define a Ruby object that stores a pointer to a struct idle_sampling_loop_state
// This structure is used to define a Ruby object that stores a pointer to a idle_sampling_loop_state
// See also https://github.com/ruby/ruby/blob/master/doc/extension.rdoc for how this works
static const rb_data_type_t idle_sampling_helper_typed_data = {
.wrap_struct_name = "Datadog::Profiling::Collectors::IdleSamplingHelper",
Expand All @@ -76,7 +76,7 @@ static const rb_data_type_t idle_sampling_helper_typed_data = {
};

static VALUE _native_new(VALUE klass) {
struct idle_sampling_loop_state *state = ruby_xcalloc(1, sizeof(struct idle_sampling_loop_state));
idle_sampling_loop_state *state = ruby_xcalloc(1, sizeof(idle_sampling_loop_state));

// Note: Any exceptions raised from this note until the TypedData_Wrap_Struct call will lead to the state memory
// being leaked.
Expand All @@ -90,7 +90,7 @@ static VALUE _native_new(VALUE klass) {
return TypedData_Wrap_Struct(klass, &idle_sampling_helper_typed_data, state);
}

static void reset_state(struct idle_sampling_loop_state *state) {
static void reset_state(idle_sampling_loop_state *state) {
state->wakeup_mutex = (pthread_mutex_t) PTHREAD_MUTEX_INITIALIZER;
state->wakeup = (pthread_cond_t) PTHREAD_COND_INITIALIZER;
state->requested_action = ACTION_WAIT;
Expand All @@ -101,17 +101,17 @@ static void reset_state(struct idle_sampling_loop_state *state) {
// a pristine state before recreating the worker thread (this includes resetting the mutex in case it was left
// locked halfway through the VM forking)
static VALUE _native_reset(DDTRACE_UNUSED VALUE self, VALUE self_instance) {
struct idle_sampling_loop_state *state;
TypedData_Get_Struct(self_instance, struct idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);
idle_sampling_loop_state *state;
TypedData_Get_Struct(self_instance, idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);

reset_state(state);

return Qtrue;
}

static VALUE _native_idle_sampling_loop(DDTRACE_UNUSED VALUE self, VALUE self_instance) {
struct idle_sampling_loop_state *state;
TypedData_Get_Struct(self_instance, struct idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);
idle_sampling_loop_state *state;
TypedData_Get_Struct(self_instance, idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);

// Release GVL and run the loop waiting for requests
rb_thread_call_without_gvl(run_idle_sampling_loop, state, interrupt_idle_sampling_loop, state);
Expand All @@ -120,7 +120,7 @@ static VALUE _native_idle_sampling_loop(DDTRACE_UNUSED VALUE self, VALUE self_in
}

static void *run_idle_sampling_loop(void *state_ptr) {
struct idle_sampling_loop_state *state = (struct idle_sampling_loop_state *) state_ptr;
idle_sampling_loop_state *state = (idle_sampling_loop_state *) state_ptr;
int error = 0;

while (true) {
Expand Down Expand Up @@ -164,7 +164,7 @@ static void *run_idle_sampling_loop(void *state_ptr) {
}

static void interrupt_idle_sampling_loop(void *state_ptr) {
struct idle_sampling_loop_state *state = (struct idle_sampling_loop_state *) state_ptr;
idle_sampling_loop_state *state = (idle_sampling_loop_state *) state_ptr;
int error = 0;

// Note about the error handling in this situation: Something bad happening at this stage is really really awkward to
Expand All @@ -189,8 +189,8 @@ static void interrupt_idle_sampling_loop(void *state_ptr) {
}

static VALUE _native_stop(DDTRACE_UNUSED VALUE self, VALUE self_instance) {
struct idle_sampling_loop_state *state;
TypedData_Get_Struct(self_instance, struct idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);
idle_sampling_loop_state *state;
TypedData_Get_Struct(self_instance, idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);

ENFORCE_SUCCESS_GVL(pthread_mutex_lock(&state->wakeup_mutex));
state->requested_action = ACTION_STOP;
Expand All @@ -204,12 +204,12 @@ static VALUE _native_stop(DDTRACE_UNUSED VALUE self, VALUE self_instance) {

// Assumption: Function gets called without the global VM lock
void idle_sampling_helper_request_action(VALUE self_instance, void (*run_action_function)(void)) {
struct idle_sampling_loop_state *state;
idle_sampling_loop_state *state;
if (!rb_typeddata_is_kind_of(self_instance, &idle_sampling_helper_typed_data)) {
grab_gvl_and_raise(rb_eTypeError, "Wrong argument for idle_sampling_helper_request_action");
}
// This should never fail the the above check passes
TypedData_Get_Struct(self_instance, struct idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);
TypedData_Get_Struct(self_instance, idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);

ENFORCE_SUCCESS_NO_GVL(pthread_mutex_lock(&state->wakeup_mutex));
if (state->requested_action == ACTION_WAIT) {
Expand Down
14 changes: 7 additions & 7 deletions ext/datadog_profiling_native_extension/collectors_stack.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@
static VALUE missing_string = Qnil;

// Used as scratch space during sampling
struct sampling_buffer {
struct sampling_buffer { // Note: typedef'd in the header to sampling_buffer
uint16_t max_frames;
ddog_prof_Location *locations;
frame_info *stack_buffer;
}; // Note: typedef'd in the header to sampling_buffer
};

static VALUE _native_sample(int argc, VALUE *argv, DDTRACE_UNUSED VALUE _self);
static VALUE native_sample_do(VALUE args);
Expand All @@ -44,15 +44,15 @@ void collectors_stack_init(VALUE profiling_module) {
rb_global_variable(&missing_string);
}

struct native_sample_args {
typedef struct {
VALUE in_gc;
VALUE recorder_instance;
sample_values values;
sample_labels labels;
VALUE thread;
ddog_prof_Location *locations;
sampling_buffer *buffer;
};
} native_sample_args;

// This method exists only to enable testing Datadog::Profiling::Collectors::Stack behavior using RSpec.
// It SHOULD NOT be used for other purposes.
Expand Down Expand Up @@ -123,7 +123,7 @@ static VALUE _native_sample(int argc, VALUE *argv, DDTRACE_UNUSED VALUE _self) {

ddog_prof_Slice_Label slice_labels = {.ptr = labels, .len = labels_count};

struct native_sample_args args_struct = {
native_sample_args args_struct = {
.in_gc = in_gc,
.recorder_instance = recorder_instance,
.values = values,
Expand All @@ -137,7 +137,7 @@ static VALUE _native_sample(int argc, VALUE *argv, DDTRACE_UNUSED VALUE _self) {
}

static VALUE native_sample_do(VALUE args) {
struct native_sample_args *args_struct = (struct native_sample_args *) args;
native_sample_args *args_struct = (native_sample_args *) args;

if (args_struct->in_gc == Qtrue) {
record_placeholder_stack(
Expand All @@ -160,7 +160,7 @@ static VALUE native_sample_do(VALUE args) {
}

static VALUE native_sample_ensure(VALUE args) {
struct native_sample_args *args_struct = (struct native_sample_args *) args;
native_sample_args *args_struct = (native_sample_args *) args;

ruby_xfree(args_struct->locations);
sampling_buffer_free(args_struct->buffer);
Expand Down
Loading

0 comments on commit 913fe43

Please sign in to comment.