[PROF-8667] Heap Profiling - Part 3 - Frees and Queue #3283
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… alexjf/prof-8667-heap-profiling-part3-free-and-queue
… alexjf/prof-8667-heap-profiling-part3-free-and-queue
ivoanjo
reviewed
Nov 30, 2023
| @@ -143,6 +163,8 @@ heap_recorder* heap_recorder_new(void) { | |||
| .obj = Qnil, | |||
There was a problem hiding this comment.
⬆️ I just noticed that for the recorder we're using xmalloc instead of xcalloc -- maybe worth using xcalloc? Just in case we forget to initialize something at some point and we get a more sane default.
Comment on lines
222
to
236
| if (error) { | ||
| // unhappy and hopefully rare path | ||
| if (error == EBUSY) { | ||
| // We weren't able to get a lock | ||
| // TODO: Add some queuing system so we can do something other than drop this data. | ||
| // We weren't able to get a lock, enqueue it for later processing | ||
| // When enqueueing, always use a new stack, we can't guarantee a reference to a shared one | ||
| // won't be cleared in-between due to processing of a free. | ||
| // We could potentially improve this but this is not supposed to be the critical path anyway. | ||
| heap_stack *stack = heap_stack_new(locations); | ||
| enqueue_allocation(heap_recorder, stack, new_obj, active_recording->object_data); | ||
| } else { | ||
| // Something unexpected happened, lets error out | ||
| ENFORCE_SUCCESS_GVL(error) | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Minor: Maybe we could structure this as
if (error == EBUSY) {
// ... stuff
return;
}
if (error) ENFORCE_SUCCESS_GVL(error);
// Happy path goes here
to avoid the weird return that's hanging after the else but actually belongs to the if side only ;)
Comment on lines
267
to
+276
| if (object_record == NULL) { | ||
| // we don't seem to be tracking this object on the table atm | ||
| // check if the allocation sample is in the queue | ||
| for (size_t i = 0; i < heap_recorder->queued_samples_len; i++) { | ||
| uncommitted_sample *queued_sample = &heap_recorder->queued_samples[i]; | ||
| if (queued_sample->obj == obj && !queued_sample->skip) { | ||
| queued_sample->skip = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure it's something we want/can do something at this point (or maybe this is another reason not to use the freeobj tracepoint), but I'm thinking this can have a potential downside in a case such as:
- The queue has a bunch of stuff to do
- Ruby starts a GC, so queue is not processed
- Ruby releases A LOT of objects, with in the extreme case none of them actually being tracked by us
- We need to go through the (potentially long) queue for every one of this objects
- Only after the GC do we process the queue
|
Replaced with #3328 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2.0 Upgrade Guide notes
What does this PR do?
Motivation:
Additional Notes:
How to test the change?
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!