[PROF-8667] Heap Profiling - Part 1 - Setup #3281

AlexJF · 2023-11-24T14:36:27Z

What does this PR do?

This PR paves the way for the introduction of heap profiling functionality by:

Introduction of a setting for controlling heap profiling (DD_PROFILING_EXPERIMENTAL_HEAP_ENABLED, false by default).
Refactoring of settings related to allocation profiling (DD_PROFILING_EXPERIMENTAL_ALLOCATION_ENABLED and DD_PROFILING_EXPERIMENTAL_ALLOCATION_SAMPLE_RATE) and improving the warnings on broken rubies.
- As a result of this refactoring, allocation counting is now tied with allocation profiling and can no longer be enabled stand-alone.
Introduction of a heap recorder component (with noop implementation for now) in the native profiling extension and plugging it in on top of the existing allocation profiling functionality.
Interaction with the heap recorder component to collect new heap-live-samples data at profile serialization time.
The necessary tests to have coverage for this functionality (some marked as pending until the proper implementation is added)

Future PRs will gradually build on the heap recorder implementation to actually enable heap data collection.

Motivation:
Start adding heap profiling capabilities in easy-to-review chunks.

Additional Notes:

How to test the change?
The only visible result of this PR is the inclusion of heap-live-samples values in the resulting Ruby profiles when an app is executed with:

DD_PROFILING_EXPERIMENTAL_ALLOCATION_ENABLEd=true DD_PROFILING_EXPERIMENTAL_HEAP_ENABLED=true ddtracerb exec ...

For Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

ivoanjo

I think this is looking great! Did a first pass but ran out of time, so here's the comments I got so far :)

ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time_worker.c

lib/datadog/core/configuration/settings.rb

lib/datadog/profiling/component.rb

ext/ddtrace_profiling_native_extension/collectors_thread_context.c

ivoanjo

Here's another full detailed pass! I'm still missing a pass at the specs, will do so asap :)

ext/ddtrace_profiling_native_extension/collectors_cpu_and_wall_time_worker.c

ext/ddtrace_profiling_native_extension/stack_recorder.c

lib/datadog/core/configuration/settings.rb

lib/datadog/profiling/component.rb

ivoanjo

Did a pass on the specs!

spec/datadog/core/configuration/settings_spec.rb

spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb

spec/datadog/profiling/spec_helper.rb

spec/datadog/profiling/stack_recorder_spec.rb

spec/datadog/profiling/component_spec.rb

ivoanjo

I've left a last round of comments. Most of them are minor, and the others are more for later than for this PR.

I think this is in good shape to go in. Here sir, have my 👍

ext/ddtrace_profiling_native_extension/stack_recorder.c

ext/ddtrace_profiling_native_extension/heap_recorder.c

spec/datadog/profiling/stack_recorder_spec.rb

ivoanjo · 2023-12-01T10:57:02Z

spec/datadog/profiling/stack_recorder_spec.rb

+        it 'do not corrupt/overwrite non-heap-samples' do
+          expect(non_heap_samples.size).to eq(2)
+
+          sum_cpu_time = 0
+          sum_alloc_samples = 0
+
+          non_heap_samples.each do |s|
+            sum_cpu_time += s.values[:'cpu-time']
+            sum_alloc_samples += s.values[:'alloc-samples']
+          end
+
+          expect(sum_cpu_time).to be > 0
+          expect(sum_alloc_samples).to eq(@num_allocations * sample_rate)
+        end


Minor: I very rarely add tests for things that don't happen, since a lot of things don't happen in most cases. I'm curious to know your thinking on why this one is worth having around?

I tend to agree with the intention in general. But in this case, because of the extra work needed to "mock" allocation tracking in these tests, this section is the only one where we actually have heap_samples_enabled.

So all the other tests in this suite that validate non-heap profiling do not cover heap_samples_enabled=true case which is also the only case where we do "out-of-band" sample writing that could, in theory, overwrite/mess with something that happens in the normal recording. The intention of this test is to serve as a canary for such a problem.

An alternative might be adding a positive test case with heap_samples_enabled=true to the existing test context?

spec/datadog/profiling/stack_recorder_spec.rb

codecov-commenter · 2023-12-11T15:26:13Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (0fa4a67) 98.23% compared to head (fad58f4) 98.23%.

Files	Patch %	Lines
spec/datadog/profiling/stack_recorder_spec.rb	95.00%	3 Missing ⚠️
lib/datadog/profiling/component.rb	97.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3281      +/-   ##
==========================================
- Coverage   98.23%   98.23%   -0.01%     
==========================================
  Files        1253     1253              
  Lines       72766    72991     +225     
  Branches     3417     3430      +13     
==========================================
+ Hits        71481    71702     +221     
- Misses       1285     1289       +4

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

ivoanjo

👍 Did a final tiny pass, I think this is ready!

ext/ddtrace_profiling_native_extension/stack_recorder.c

ivoanjo · 2023-12-12T12:34:53Z

YAY!

**What does this PR do?** This PR introduces a new setting for the profiler: `profiling.advanced.allocation_counting_enabled` which controls the profiler allocation counting feature. This setting is off by default, enabling us to reduce allocation profiling overhead slightly. **Motivation:** We actually used to have this setting back in the ddtrace 1.x series. We introduced it in #2635 and removed it in #3281 -- by tieing it directly to allocation profiling. I decided to re-introduce it so we can disable this feature by default. **Additional Notes:** This PR sits atop #3797 because it makes sense to measure both overhead improvements together, but is otherwise completely independent. **How to test the change?** Here's the results for running the `profiler_allocation` benchmark: ``` ruby 2.7.7p221 (2022-11-24 revision 168ec2b1e5) [x86_64-linux] Warming up -------------------------------------- Allocations (baseline) 1.419M i/100ms Calculating ------------------------------------- Allocations (baseline) 14.535M (± 2.1%) i/s - 146.197M in 10.062717s Warming up -------------------------------------- Allocations (alloc_profiling_enabled) 1.122M i/100ms Calculating ------------------------------------- Allocations (alloc_profiling_enabled) 11.636M (± 2.2%) i/s - 116.679M in 10.032209s Warming up -------------------------------------- Allocations (alloc_profiling_disabled) 1.124M i/100ms Calculating ------------------------------------- Allocations (alloc_profiling_disabled) 11.866M (± 2.6%) i/s - 119.175M in 10.050580s Comparison: Allocations (baseline): 14534979.3 i/s Allocations (alloc_profiling_disabled): 11865926.7 i/s - 1.22x slower Allocations (alloc_profiling_enabled): 11635919.9 i/s - 1.25x slower ``` The difference is close to the margin of error; nevertheless this feature was showing up on the native profiler, and since it was on the hot path for allocation profiling, I think it's worth it.

[PROF-8667] Heap Profiling - Part 1 - Setup

a62fc7a

github-actions bot added core Involves Datadog core libraries profiling Involves Datadog profiling labels Nov 24, 2023

[PROF-8667] Fix CI

6cff321

ivoanjo reviewed Nov 24, 2023

View reviewed changes

AlexJF added 5 commits November 24, 2023 17:28

[PROF-8667] More CI fixes

043b3e7

[PROF-8667] Address initial comments, more tests and more test fixes.

04b4f01

[PROF-8667] Slightly simpler skipping of after on initialization error

350058f

[PROF-8667] Fix two extra issues with CI checks

4614b2b

[PROF-8667] One more missing filter->select.

e61e72e

ivoanjo reviewed Nov 28, 2023

View reviewed changes

[PROF-8667] More comment addressing

b939766

AlexJF force-pushed the alexjf/prof-8667-heap-profiling-part1 branch from 0cb2a47 to b939766 Compare November 30, 2023 12:33

AlexJF added 6 commits November 30, 2023 12:37

[PROF-8667] Missing comment

e94b260

[PROF-8667] Migrate to an approach with no free tracepointing.

f51c254

[PROF-8667] Fix types

6893e74

[PROF-8667] Do not use sum for max Ruby compat.

21bee6a

Merge branch 'master' into alexjf/prof-8667-heap-profiling-part1

242a4ad

[PROF-8667] Remove one remaining usage of sum.

56745c9

ivoanjo approved these changes Dec 1, 2023

View reviewed changes

ivoanjo mentioned this pull request Dec 8, 2023

[NO-TICKET] Redesign GC profiling to add timeline support and reduce overhead #3313

Merged

2 tasks

AlexJF added 3 commits December 11, 2023 12:16

Merge branch 'master' into alexjf/prof-8667-heap-profiling-part1

c55868f

[PROF-8667] Remove last traces of object_free_tracepoint.

3ac74e0

[PROF-8667] Final cleanup and comment addressing

fdca793

AlexJF marked this pull request as ready for review December 11, 2023 14:32

AlexJF requested review from a team as code owners December 11, 2023 14:32

[PROF-8667] Fix lint error.

fad58f4

ivoanjo approved these changes Dec 12, 2023

View reviewed changes

ext/ddtrace_profiling_native_extension/stack_recorder.c Outdated Show resolved Hide resolved

ext/ddtrace_profiling_native_extension/stack_recorder.c Show resolved Hide resolved

[PROF-8667] Last comment and more thorough checking in a test case

3ec2698

AlexJF merged commit dfbc324 into master Dec 12, 2023
218 checks passed

AlexJF deleted the alexjf/prof-8667-heap-profiling-part1 branch December 12, 2023 12:28

github-actions bot added this to the 1.19.0 milestone Dec 12, 2023

This was referenced Dec 13, 2023

[PROF-8543] Ruby heap profiling PoC #3246

Closed

[PROF-8632] Ruby heap size profiling PoC #3261

Closed

[PROF-8648] Add labels to heap samples #3263

Closed

[PROF-8667] Heap Profiling - Part 2 - Heap Recorder #3287

Merged

ivoanjo mentioned this pull request Jul 22, 2024

[PROF-10201] Disable profiler allocation counting feature by default #3798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PROF-8667] Heap Profiling - Part 1 - Setup #3281

[PROF-8667] Heap Profiling - Part 1 - Setup #3281

AlexJF commented Nov 24, 2023 •

edited

Loading

ivoanjo left a comment

ivoanjo left a comment

ivoanjo left a comment

ivoanjo left a comment

ivoanjo Dec 1, 2023

AlexJF Dec 11, 2023

codecov-commenter commented Dec 11, 2023

ivoanjo left a comment

ivoanjo commented Dec 12, 2023

[PROF-8667] Heap Profiling - Part 1 - Setup #3281

[PROF-8667] Heap Profiling - Part 1 - Setup #3281

Conversation

AlexJF commented Nov 24, 2023 • edited Loading

ivoanjo left a comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

ivoanjo Dec 1, 2023

Choose a reason for hiding this comment

AlexJF Dec 11, 2023

Choose a reason for hiding this comment

codecov-commenter commented Dec 11, 2023

Codecov Report

ivoanjo left a comment

Choose a reason for hiding this comment

ivoanjo commented Dec 12, 2023

AlexJF commented Nov 24, 2023 •

edited

Loading