Skip to content

Commit 913fe43

Browse files
committed
[NO-TICKET] Profiling refactor: typedef most structures
**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.
1 parent b26f306 commit 913fe43

15 files changed

+270
-270
lines changed

ext/datadog_profiling_native_extension/clock_id.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@
55
#include <ruby.h>
66

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

1313
// Contains the current cpu time, and a flag to indicate if we failed to fetch it
14-
typedef struct thread_cpu_time {
14+
typedef struct {
1515
bool valid;
1616
long result_ns;
1717
} thread_cpu_time;

ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c

Lines changed: 54 additions & 54 deletions
Large diffs are not rendered by default.

ext/datadog_profiling_native_extension/collectors_discrete_dynamic_sampler.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ static VALUE _native_should_sample(VALUE self, VALUE now);
333333
static VALUE _native_after_sample(VALUE self, VALUE now);
334334
static VALUE _native_state_snapshot(VALUE self);
335335

336-
typedef struct sampler_state {
336+
typedef struct {
337337
discrete_dynamic_sampler sampler;
338338
} sampler_state;
339339

ext/datadog_profiling_native_extension/collectors_discrete_dynamic_sampler.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
// every event and is thus, in theory, susceptible to some pattern
1717
// biases. In practice, the dynamic readjustment of sampling interval
1818
// and randomized starting point should help with avoiding heavy biases.
19-
typedef struct discrete_dynamic_sampler {
19+
typedef struct {
2020
// --- Config ---
2121
// Name of this sampler for debug logs.
2222
const char *debug_name;

ext/datadog_profiling_native_extension/collectors_idle_sampling_helper.c

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,15 @@
2121
typedef enum { ACTION_WAIT, ACTION_RUN, ACTION_STOP } action;
2222

2323
// Contains state for a single CpuAndWallTimeWorker instance
24-
struct idle_sampling_loop_state {
24+
typedef struct {
2525
pthread_mutex_t wakeup_mutex;
2626
pthread_cond_t wakeup;
2727
action requested_action;
2828
void (*run_action_function)(void);
29-
};
29+
} idle_sampling_loop_state;
3030

3131
static VALUE _native_new(VALUE klass);
32-
static void reset_state(struct idle_sampling_loop_state *state);
32+
static void reset_state(idle_sampling_loop_state *state);
3333
static VALUE _native_idle_sampling_loop(DDTRACE_UNUSED VALUE self, VALUE self_instance);
3434
static VALUE _native_stop(DDTRACE_UNUSED VALUE self, VALUE self_instance);
3535
static void *run_idle_sampling_loop(void *state_ptr);
@@ -62,7 +62,7 @@ void collectors_idle_sampling_helper_init(VALUE profiling_module) {
6262
rb_define_singleton_method(testing_module, "_native_idle_sampling_helper_request_action", _native_idle_sampling_helper_request_action, 1);
6363
}
6464

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

7878
static VALUE _native_new(VALUE klass) {
79-
struct idle_sampling_loop_state *state = ruby_xcalloc(1, sizeof(struct idle_sampling_loop_state));
79+
idle_sampling_loop_state *state = ruby_xcalloc(1, sizeof(idle_sampling_loop_state));
8080

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

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

107107
reset_state(state);
108108

109109
return Qtrue;
110110
}
111111

112112
static VALUE _native_idle_sampling_loop(DDTRACE_UNUSED VALUE self, VALUE self_instance) {
113-
struct idle_sampling_loop_state *state;
114-
TypedData_Get_Struct(self_instance, struct idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);
113+
idle_sampling_loop_state *state;
114+
TypedData_Get_Struct(self_instance, idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);
115115

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

122122
static void *run_idle_sampling_loop(void *state_ptr) {
123-
struct idle_sampling_loop_state *state = (struct idle_sampling_loop_state *) state_ptr;
123+
idle_sampling_loop_state *state = (idle_sampling_loop_state *) state_ptr;
124124
int error = 0;
125125

126126
while (true) {
@@ -164,7 +164,7 @@ static void *run_idle_sampling_loop(void *state_ptr) {
164164
}
165165

166166
static void interrupt_idle_sampling_loop(void *state_ptr) {
167-
struct idle_sampling_loop_state *state = (struct idle_sampling_loop_state *) state_ptr;
167+
idle_sampling_loop_state *state = (idle_sampling_loop_state *) state_ptr;
168168
int error = 0;
169169

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

191191
static VALUE _native_stop(DDTRACE_UNUSED VALUE self, VALUE self_instance) {
192-
struct idle_sampling_loop_state *state;
193-
TypedData_Get_Struct(self_instance, struct idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);
192+
idle_sampling_loop_state *state;
193+
TypedData_Get_Struct(self_instance, idle_sampling_loop_state, &idle_sampling_helper_typed_data, state);
194194

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

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

214214
ENFORCE_SUCCESS_NO_GVL(pthread_mutex_lock(&state->wakeup_mutex));
215215
if (state->requested_action == ACTION_WAIT) {

ext/datadog_profiling_native_extension/collectors_stack.c

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,11 @@
1414
static VALUE missing_string = Qnil;
1515

1616
// Used as scratch space during sampling
17-
struct sampling_buffer {
17+
struct sampling_buffer { // Note: typedef'd in the header to sampling_buffer
1818
uint16_t max_frames;
1919
ddog_prof_Location *locations;
2020
frame_info *stack_buffer;
21-
}; // Note: typedef'd in the header to sampling_buffer
21+
};
2222

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

47-
struct native_sample_args {
47+
typedef struct {
4848
VALUE in_gc;
4949
VALUE recorder_instance;
5050
sample_values values;
5151
sample_labels labels;
5252
VALUE thread;
5353
ddog_prof_Location *locations;
5454
sampling_buffer *buffer;
55-
};
55+
} native_sample_args;
5656

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

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

126-
struct native_sample_args args_struct = {
126+
native_sample_args args_struct = {
127127
.in_gc = in_gc,
128128
.recorder_instance = recorder_instance,
129129
.values = values,
@@ -137,7 +137,7 @@ static VALUE _native_sample(int argc, VALUE *argv, DDTRACE_UNUSED VALUE _self) {
137137
}
138138

139139
static VALUE native_sample_do(VALUE args) {
140-
struct native_sample_args *args_struct = (struct native_sample_args *) args;
140+
native_sample_args *args_struct = (native_sample_args *) args;
141141

142142
if (args_struct->in_gc == Qtrue) {
143143
record_placeholder_stack(
@@ -160,7 +160,7 @@ static VALUE native_sample_do(VALUE args) {
160160
}
161161

162162
static VALUE native_sample_ensure(VALUE args) {
163-
struct native_sample_args *args_struct = (struct native_sample_args *) args;
163+
native_sample_args *args_struct = (native_sample_args *) args;
164164

165165
ruby_xfree(args_struct->locations);
166166
sampling_buffer_free(args_struct->buffer);

0 commit comments

Comments
 (0)