[PROF-7440] Add approximate thread state categorization for timeline #3162
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,13 +2,21 @@ | |
|
|
||
| #include <datadog/profiling.h> | ||
|
|
||
| typedef struct { | ||
| int64_t cpu_time_ns; | ||
| int64_t wall_time_ns; | ||
| uint32_t cpu_or_wall_samples; | ||
| uint32_t alloc_samples; | ||
| } sample_values; | ||
|
|
||
| typedef struct sample_labels { | ||
| ddog_prof_Slice_Label labels; | ||
|
|
||
| // This is used to allow the `Collectors::Stack` to modify the existing label, if any. This MUST be NULL or point | ||
| // somewhere inside the labels slice above. | ||
|
Comment on lines
+15
to
+16
There was a problem hiding this comment. Nit: Feels a bit weird to have this "pollute" the model/prototypes when it seems to be an internal detail of Addendum: After progressing in the review, noticed you do have the sanity check of checking state label exists if it's a CPU/wall sample so I guess we'd be forced to look the label up for almost every sample which may justify the "caching" done here so I have less of an issue with it now and leaving this here just to make sure I understood intentions 😄 There was a problem hiding this comment. You got it exactly right. This label was a bit of a new thing, because up until now all labels were set by the thread context collector, and they were opaque to everyone else. But now suddenly we have this need to have a label that shows up based on the stack, which with the current structure of the profiler happens later than all the other labels. And yeah, this is something that will happen on every cpu/wall sample, for every thread, with the Ruby VM stopped for sampling, so it's on the hot path and thus we'd need to be careful if we wanted to abstract this to avoid overhead. So that's why it ended up like this. As a follow-up note, it doesn't 100% need to be like this. Currently, the array for the labels is stack allocated in the thread context collector, and it's sized to fit exactly the number of labels we need. If we were to stop stack-allocating it, and instead had a pre-sized buffer that was a bit bigger, we could then keep a pointer to the next free element and the size of the buffer or something like that, and the stack collector could then just add the label when it wants. (Technically we could do that with the stack-allocated array, and my first prototype of this did that, but I didn't quite like how it looked) The stack collector already has a number of such fixed buffers to store the raw stack sample it gets from the VM, and indeed in a past prototype they started as stack-allocated and then moved to reusable heap-allocated buffers. Furthermore, if we moved more stuff away from stack allocation, we could have a bit more freedom in sequencing the operations in a sample, e.g. right now we have collectors calling each other as shown in https://github.com/DataDog/dd-trace-rb/blob/master/docs/ProfilingDevelopment.md#how-sampling-happens , but we could move to a more flat approach: sample the_sample = ...
thread_context.add_labels(the_sample)
thread_context.add_values(the_sample)
stack.add_stack(the_sample)
...
recorder.record(the_sample)So yeah... Happy to revisit these things as you find them :) |
||
| ddog_prof_Label *state_label; | ||
| } sample_labels; | ||
|
|
||
| void record_sample(VALUE recorder_instance, ddog_prof_Slice_Location locations, sample_values values, sample_labels labels); | ||
| void record_endpoint(VALUE recorder_instance, uint64_t local_root_span_id, ddog_CharSlice endpoint); | ||
| VALUE enforce_recorder_instance(VALUE object); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have an explicit concat with
""in the strlen part but no such concat in thestrncmp. I'd expect to need one in both places or neither, what's the deal here? 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a bit of C dark magic cool trick. It relies on the fact that you can do this in C:
And the result is concatenated. Note that you can only do this for literals, that is, it's not a generic way of joining strings in C.
In this case, I wanted to force the string being compared to the charslice to be a literal, so by by doing
This causes the compiler to barf if you do this:
Output:
The error could be a bit better but I hope the "must_be_a_literal" hint would lead people in the right direction ;)
The concat doesn't need to happen on both because
"" must_be_a_literaldoesn't do anything else other than cause the error to happen, and otherwise is a no-op. (Doing it on both causes more errors on the compiler side, so from that POV is even slightly worse)As a final note, why I did this: more out of a tiny cheap micro-optimization; by forcing it to be a literal, I know the compiler can optimize away the
strlen; otherwise I would probably have wanted to cache it somewhere, which would be more boilerplate.