Skip to content

Make sample indexes compatible between the unfiltered and (preview) filtered call tree summary strategy samples when using an allocation strategy#5332

Merged
mstange merged 3 commits into
firefox-devtools:mainfrom
mstange:consistent-allocation-sample-indexes
Jan 20, 2025
Merged

Make sample indexes compatible between the unfiltered and (preview) filtered call tree summary strategy samples when using an allocation strategy#5332
mstange merged 3 commits into
firefox-devtools:mainfrom
mstange:consistent-allocation-sample-indexes

Conversation

@mstange

@mstange mstange commented Jan 17, 2025

Copy link
Copy Markdown
Contributor

Production | Deploy preview

Fixes #5331.

Let's say we allocate 100 objects, and then free the first 50 of them. If you look at the entire thread, the retained-native-allocations view will show the allocation samples for the second 50 objects (the ones that weren't freed).
If you look at just the part of the thread where you allocated, you will see allocation samples for all 100 objects (because none were freed in that part of the thread).
How do you get the stack category for each of the 100 allocation samples, especially if you've applied transforms or a JS-only filter?

You need to look at the stack of the raw nativeAllocations table.

You cannot look at the retained-native-allocations of the entire thread, because that table doesn't have the stacks for the first 50 objects - it removes those samples because it knows that those objects have been deallocated.

This commit makes it so that the unfilteredCtssSamples are just the raw allocation tables, when an allocation-related call tree summary strategy is selected.

And it makes it so that extractSamplesLikeTable always returns a table that has compatible indexes to the extractUnfilteredCtssSamples, just with some samples having a null stack.

Now the unfilteredCtssSamples can be used to look up the stack, and therefore the original category of a sample, based on the sample index in the filtered (or preview-filtered) CTSS samples table. We should not create a call tree just based on the unfilteredCtssSamples because these samples have not been filtered according to the strategy.

@mstange mstange requested a review from julienw January 17, 2025 21:04
@mstange mstange self-assigned this Jan 17, 2025
@mstange

mstange commented Jan 17, 2025

Copy link
Copy Markdown
Contributor Author

I briefly looked at adding a test for this but it was too much work.

@codecov

codecov Bot commented Jan 17, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.07%. Comparing base (5431546) to head (0bf47ed).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5332      +/-   ##
==========================================
- Coverage   86.08%   86.07%   -0.02%     
==========================================
  Files         311      311              
  Lines       29687    29654      -33     
  Branches     8192     8194       +2     
==========================================
- Hits        25557    25525      -32     
+ Misses       3548     3547       -1     
  Partials      582      582              

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

@mstange mstange force-pushed the consistent-allocation-sample-indexes branch from 7bdeccd to 893985e Compare January 17, 2025 21:30
…iltered call tree summary strategy samples when using an allocation strategy.

Fixes firefox-devtools#5331.

Let's say we allocate 100 objects, and then free the first 50 of them.
If you look at the entire thread, the `retained-native-allocations` view
will show the allocation samples for the second 50 objects (the ones
that weren't freed).
If you look at just the part of the thread where you allocated, you
will see allocation samples for all 100 objects (because none were
freed in that part of the thread).

How do you get the stack category for each of the 100 allocation samples,
especially if you've applied transforms or a JS-only filter?

You need to look at the stack of the raw nativeAllocations table.

You cannot look at the `retained-native-allocations` of the entire
thread, because that table doesn't have the stacks for the first
50 objects - it removes those samples because it knows that those
objects have been deallocated.

This commit makes it so that the unfilteredCtssSamples are just
the raw allocation tables, when an allocation-related call tree
summary strategy is selected.

And it makes it so that `extractSamplesLikeTable` always returns
a table that has compatible indexes to the `extractUnfilteredCtssSamples`,
just with some samples having a null stack.

Now the unfilteredCtssSamples can be used to look up the stack, and
therefore the original category of a sample, based on the sample
index in the filtered (or preview-filtered) CTSS samples table. We
should not create a call tree just based on the unfilteredCtssSamples
because these samples have not been filtered according to the strategy.
@mstange mstange force-pushed the consistent-allocation-sample-indexes branch from 893985e to 0bf47ed Compare January 17, 2025 21:30

@julienw julienw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense to me, thanks for the fix.

I notice something else with your deploy preview, in the flame graph this time:

See below: when moving the preview selection, at one point the flame graph doesn't change even though I believe the proportions should still change:

Capture.video.du.2025-01-20.14-50-45.webm

The behavior is the same before/after your patch so it's unrelated to this issue, but maybe there's a similar issue elsewhere. In case you have an idea.

Comment thread src/profile-logic/call-tree.js Outdated
Comment thread src/profile-logic/profile-data.js
@mstange mstange enabled auto-merge January 20, 2025 16:24
@mstange mstange merged commit 98a364a into firefox-devtools:main Jan 20, 2025
@mstange

mstange commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

See below: when moving the preview selection, at one point the flame graph doesn't change even though I believe the proportions should still change:

I think the behavior is actually correct. The proportions should not change at that point. The new green samples that you're adding to the range at the right are deallocation samples, and they're deallocating memory addresses to the left of the start of your range. So those new green samples shouldn't change which samples are shown in the call tree.

@julienw

julienw commented Jan 20, 2025

Copy link
Copy Markdown
Contributor

See below: when moving the preview selection, at one point the flame graph doesn't change even though I believe the proportions should still change:

I think the behavior is actually correct. The proportions should not change at that point. The new green samples that you're adding to the range at the right are deallocation samples, and they're deallocating memory addresses to the left of the start of your range. So those new green samples shouldn't change which samples are shown in the call tree.

OK, I agree for the first part of the video, but what about the final part, where the left handle is moved towards the left?

@mstange

mstange commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

OK, I agree for the first part of the video, but what about the final part, where the left handle is moved towards the left?

Ah, that's when you're adding allocation samples for which the (previously unpaired) deallocation samples are already in the range.

@julienw

julienw commented Jan 20, 2025

Copy link
Copy Markdown
Contributor

Ah yeah, that's right. Thanks for your patience 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar category breakdown behaves oddly when looking at allocation call trees

2 participants