Skip to content

[PROF-9469] Skip heap samples with age 0#3573

Merged
AlexJF merged 5 commits into
masterfrom
alexjf/prof-9469-skip-age-0-samples
Apr 10, 2024
Merged

[PROF-9469] Skip heap samples with age 0#3573
AlexJF merged 5 commits into
masterfrom
alexjf/prof-9469-skip-age-0-samples

Conversation

@AlexJF

@AlexJF AlexJF commented Apr 4, 2024

Copy link
Copy Markdown
Contributor

What does this PR do?

Skip updating and serialization of heap samples for objects that were allocated in the current GC epoch/generation. The fact that these objects are still alive is usually just a happy accident from the fact that GC hasn't run yet and thus just add noise when you want to understand long-term heap usage and, more specifically, memory leaks.

In fact, allocation profiles alone can give you a rather accurate description of what the population of age == 0 should look like at any-one time. If no GCs ever ran, allocation profiles would be === to heap profiles as all objects would be technically alive.

Motivation:

  • Objects with age == 0 tend to be mostly noise when investigating heap usage.
  • Unless GCs occur at intervals <1 minute or you have big memory leaks, objects with age == 0 will naturally be much more common than objects with age > 0 and so end up being major contributors to sampling and serialization overhead. Skipping them can result in significant savings.

Additional Notes:

Some micro-benchmark results:

Comparison:
sample+serialize min_age_1 retain_every=10 heap_samples=true heap_size=true heap_sample_every=10 skip_end_gc=true:        5.7 i/s
sample+serialize min_age_0 retain_every=10 heap_samples=true heap_size=true heap_sample_every=10 skip_end_gc=true:        5.5 i/s - 1.02x  slower
sample+serialize min_age_1 retain_every=10 heap_samples=true heap_size=true heap_sample_every=1 skip_end_gc=true:        4.3 i/s - 1.33x  slower
sample+serialize min_age_0 retain_every=10 heap_samples=true heap_size=true heap_sample_every=1 skip_end_gc=true:        3.1 i/s - 1.85x  slower

How to test the change?

Unsure? Have a question? Request a review!

@github-actions github-actions Bot added the profiling Involves Datadog profiling label Apr 4, 2024
@AlexJF
AlexJF force-pushed the alexjf/prof-9469-skip-age-0-samples branch 2 times, most recently from ca576a9 to cc459c7 Compare April 4, 2024 17:23
@AlexJF
AlexJF force-pushed the alexjf/prof-9469-skip-age-0-samples branch from cc459c7 to f7989e4 Compare April 5, 2024 10:13
@AlexJF
AlexJF marked this pull request as ready for review April 5, 2024 10:34
@AlexJF
AlexJF requested review from a team as code owners April 5, 2024 10:34
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.27%. Comparing base (f6b234f) to head (f7989e4).
Report is 19 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3573      +/-   ##
==========================================
- Coverage   98.27%   98.27%   -0.01%     
==========================================
  Files        1274     1274              
  Lines       75233    75247      +14     
  Branches     3545     3545              
==========================================
+ Hits        73934    73947      +13     
- Misses       1299     1300       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ivoanjo ivoanjo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! I left my usual smattering of minor suggestions, one bigger suggestion and one maybe-a-bug note ;)

Comment thread ext/datadog_profiling_native_extension/stack_recorder.c Outdated
Comment thread ext/datadog_profiling_native_extension/heap_recorder.h Outdated
Comment thread ext/datadog_profiling_native_extension/heap_recorder.c Outdated
Comment thread ext/datadog_profiling_native_extension/heap_recorder.c Outdated
Comment thread ext/datadog_profiling_native_extension/heap_recorder.c
Comment thread ext/datadog_profiling_native_extension/heap_recorder.c Outdated
Comment thread spec/datadog/profiling/stack_recorder_spec.rb

@ivoanjo ivoanjo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 Looks great!

I'm slightly curious if the change moved the micro-benchmark results needle, but feel free to ignore my curiosity haha

Comment thread ext/datadog_profiling_native_extension/heap_recorder.c Outdated
@AlexJF
AlexJF force-pushed the alexjf/prof-9469-skip-age-0-samples branch from 0cbb007 to deaccf7 Compare April 9, 2024 16:14
@AlexJF

AlexJF commented Apr 9, 2024

Copy link
Copy Markdown
Contributor Author

I'm slightly curious if the change moved the micro-benchmark results needle, but feel free to ignore my curiosity haha

Did a local run and saw

Comparison:
sample+serialize min_age_1 retain_every=10 heap_samples=true heap_size=true heap_sample_every=10 skip_end_gc=true:        5.7 i/s
sample+serialize min_age_0 retain_every=10 heap_samples=true heap_size=true heap_sample_every=10 skip_end_gc=true:        5.2 i/s - 1.08x  slowe
r
sample+serialize min_age_1 retain_every=10 heap_samples=true heap_size=true heap_sample_every=1 skip_end_gc=true:        4.0 i/s - 1.40x  slower
sample+serialize min_age_0 retain_every=10 heap_samples=true heap_size=true heap_sample_every=1 skip_end_gc=true:        3.0 i/s - 1.91x  slower

which looks similar enough to the original in the PR description

@ivoanjo

ivoanjo commented Apr 10, 2024

Copy link
Copy Markdown
Member

Ah! Somewhat sad, I was hoping this tweak would buy is a bit more headroom. I guess it might still on real-world usecases, but would've been cool to see in the microbenchmark as well :)

@AlexJF
AlexJF merged commit 472ebf0 into master Apr 10, 2024
@AlexJF
AlexJF deleted the alexjf/prof-9469-skip-age-0-samples branch April 10, 2024 14:03
@github-actions github-actions Bot added this to the 1.22.0 milestone Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

profiling Involves Datadog profiling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants