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.

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

[PROF-10125] Track unscaled allocation counts in allocation profiler #3770

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jul 9, 2024

What does this PR do?

This PR extends the allocation profiler to also track the unscaled allocation counts.

Specifically, like other profile types, the allocation profiler assigns a weight to every sample, making it "represent" all the objects that weren't sampled.

Motivation:

For debugging, in corner cases, it comes in handy to know exactly how many samples the profiler observed, and what was the impact of scaling.

As part of preparing the allocation profiler feature for GA, I'm adding this feature so we can use it to confirm the exact sample counts whenever needed.

Note also this is something we do for the cpu/wall-time profiler, where we track the cpu_or_wall_samples as an exact count of how many samples were taken (and again, without the weight -- which in the case of those profilers, represents time).

Additional Notes:

N/A

How to test the change?

This change includes test coverage.

**What does this PR do?**

This PR extends the allocation profiler to also track the unscaled
allocation counts.

Specifically, like other profile types, the allocation profiler assigns
a weight to every sample, making it "represent" all the objects that
weren't sampled.

**Motivation:**

For debugging, in corner cases, it comes in handy to know exactly how
many samples the profiler observed, and what was the impact of scaling.

As part of preparing the allocation profiler feature for GA, I'm adding
this feature so we can use it to confirm the exact sample counts
whenever needed.

Note also this is something we do for the cpu/wall-time profiler, where
we track the `cpu_or_wall_samples` as an exact count of how many samples
were taken (and again, without the weight -- which in the case of those
profilers, represents time).

**Additional Notes:**

N/A

**How to test the change?**

This change includes test coverage.
@ivoanjo ivoanjo requested a review from a team as a code owner July 9, 2024 14:04
@github-actions github-actions bot added the profiling Involves Datadog profiling label Jul 9, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.91%. Comparing base (ca006e9) to head (87eeb0f).
Report is 10 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3770   +/-   ##
=======================================
  Coverage   97.91%   97.91%           
=======================================
  Files        1243     1243           
  Lines       74763    74829   +66     
  Branches     3608     3610    +2     
=======================================
+ Hits        73205    73270   +65     
- Misses       1558     1559    +1     

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

@ivoanjo ivoanjo merged commit d9912ed into master Jul 9, 2024
170 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/prof-10125-add-unscaled-allocation-counts branch July 9, 2024 15:21
@github-actions github-actions bot added this to the 2.2.0 milestone Jul 9, 2024
Comment on lines +250 to +257
{
'cpu-time' => 123,
'cpu-samples' => 456,
'wall-time' => 789,
'alloc-samples' => 4242,
'alloc-samples-unscaled' => 2222,
'timeline' => 1111,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Static Analysis Violation (ruby-best-practices/symbols-as-keys)

⚪ Notice - Best Practices - Link to Results

Consider using symbols instead of string hash keys

More information

In Ruby, it is a best practice to use symbols instead of strings as hash keys. This rule emphasizes that it's more efficient and idiomatic to use symbols for this purpose. Symbols are immutable and unique, which makes them ideal for identifying things, whereas strings are mutable and can create multiple objects for the same sequence of characters.

The importance of this rule lies in the performance and memory usage of your Ruby application. Using symbols as hash keys reduces memory usage because they are stored in memory only once during a Ruby process. This can make a significant difference in the efficiency of your application, especially when dealing with large data sets.

To ensure you're following good coding practices, always use symbols for hash keys unless there's a specific reason to use a string. A simple refactoring from values = { 'foo' => 42, 'bar' => 99, 'baz' => 123 } to values = { foo: 42, bar: 99, baz: 123 } will make your code compliant with this rule. This not only improves your code's performance but also makes it more readable and consistent with Ruby's conventions.

At the moment, we cannot generate an appropriate fix for this violation.

Leave feedback in #static-analysis

@@ -526,7 +539,7 @@
# We use the same metric_values in all sample calls in before. So we'd expect
# the summed values to match `@num_allocations * metric_values[profile-type]`
# for each profile-type there in.
expected_summed_values = { :'heap-live-samples' => 0, :'heap-live-size' => 0, }
expected_summed_values = { 'heap-live-samples': 0, 'heap-live-size': 0, 'alloc-samples-unscaled': 0 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Static Analysis Violation (ruby-best-practices/symbols-as-keys)

⚪ Notice - Best Practices - Link to Results

Consider using symbols instead of string hash keys

More information

In Ruby, it is a best practice to use symbols instead of strings as hash keys. This rule emphasizes that it's more efficient and idiomatic to use symbols for this purpose. Symbols are immutable and unique, which makes them ideal for identifying things, whereas strings are mutable and can create multiple objects for the same sequence of characters.

The importance of this rule lies in the performance and memory usage of your Ruby application. Using symbols as hash keys reduces memory usage because they are stored in memory only once during a Ruby process. This can make a significant difference in the efficiency of your application, especially when dealing with large data sets.

To ensure you're following good coding practices, always use symbols for hash keys unless there's a specific reason to use a string. A simple refactoring from values = { 'foo' => 42, 'bar' => 99, 'baz' => 123 } to values = { foo: 42, bar: 99, baz: 123 } will make your code compliant with this rule. This not only improves your code's performance but also makes it more readable and consistent with Ruby's conventions.

At the moment, we cannot generate an appropriate fix for this violation.

Leave feedback in #static-analysis

@TonyCTHsu TonyCTHsu mentioned this pull request Jul 11, 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