Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[PROF-10128] Refactor and speed up profiler stack sampling #3837

Merged
merged 17 commits into from
Aug 13, 2024
+177 −210
Merged

[PROF-10128] Refactor and speed up profiler stack sampling #3837

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d5062c5
Minor: Make argument name in header match actual function
ivoanjo Aug 8, 2024
9c61d60
Add helper function to clear per_thread_context
ivoanjo Aug 8, 2024
3d4b7fc
Remove sampling_buffer argument from record_placeholder_stack
ivoanjo Aug 8, 2024
5fd0460
Extract checking max_frames into a separate function
ivoanjo Aug 8, 2024
dc59c0a
Make sampling_buffers per-thread
ivoanjo Aug 8, 2024
99d5915
Avoid allocating locations for every sampling_buffer/thread
ivoanjo Aug 8, 2024
05e552c
Use smaller type for max_frames
ivoanjo Aug 8, 2024
71839e0
Avoid repeating call to rb_profile_frame_path for cframes
ivoanjo Aug 8, 2024
ca04090
Avoid retrieving rb_callable_method_entry_t in ddtrace_rb_profile_frames
ivoanjo Aug 8, 2024
9039572
Introduce `frame_info` to contain sampled stack frame information
ivoanjo Aug 8, 2024
47044b0
Introduce caching for sampled stacks
ivoanjo Aug 8, 2024
360aba2
Use cheaper `RSTRING_PTR` when converting string to char slice
ivoanjo Aug 8, 2024
be447a9
Fix build/running with DDTRACE_DEBUG
ivoanjo Aug 8, 2024
c09aaae
Directly use VM iseq query functions instead of going through rb_prof…
ivoanjo Aug 8, 2024
2fc03d5
Add workaround for spec that started flaking
ivoanjo Aug 9, 2024
e6c9122
Clarify why code is being left commented out
ivoanjo Aug 12, 2024
607d5d5
Pick up upstream fix for `start` parameter
ivoanjo Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ext/datadog_profiling_native_extension/heap_recorder.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,7 @@ VALUE object_record_inspect(object_record *record) {
if (!ruby_ref_from_id(LONG2NUM(record->obj_id), &ref)) {
rb_str_catf(inspect, "object=<invalid>");
} else {
rb_str_catf(inspect, "value=%p ", (void *) ref);
VALUE ruby_inspect = ruby_safe_inspect(ref);
if (ruby_inspect != Qnil) {
rb_str_catf(inspect, "object=%"PRIsVALUE, ruby_inspect);
Expand Down
11 changes: 11 additions & 0 deletions spec/datadog/profiling/stack_recorder_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,17 @@ def sample_allocation(obj)
end
GC.start # All dead objects above will be GCed, all living strings will have age = 0

# @ivoanjo: For some weird reason, the last object sampled in the "dead_heap_samples" does not always
# get collected the first time, leading to a flaky spec.
# I was able to reproduce it with `rspec spec/datadog/profiling --seed 48141 --fail-fast` and it's
# kinda bizarre since e.g. if you add one more `Object.new` it also stops flaking, so is it perhaps
# related to Ruby's conservative GC?
# I've bisected this and undoing 3d4b7fcf30b529b191ca737ae13629eb27b8ab63 also makes the flakiness
# go away, but again, that change doesn't seem to have anything to do with GC.
# As the weird behavior is transitory, e.g. it provably goes away on the next GC, I'll go with this
# workaround for now.
Comment on lines +1035 to +1038
Copy link
Contributor

@AlexJF AlexJF Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I seem to remember Ruby taking into account register values in its heuristic to detect potential usages of objects. May just be that with the extra caching of things like iseq and what not we make it slightly more likely to hit these edge cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is my suspicion -- Ruby's GC is conservative, in that if anything "looks" like a VALUE, it assumes it is. Actually debugging this would mean going into the GC marking code and understand why exactly it's making this decision which is somewhat of a time investment which is why I took the slightly-easier way out 😅.

GC.start

begin
# Allocate some extra objects in a block with GC disabled and ask for a serialization
# to ensure these strings have age=0 by the time we try to serialize the profile
Expand Down
Loading