Conversation
Instead of not computing the information in getTimingsForCallNodeIndex, compute the information anyway but then choose to not display it in the various places where we display call node timings. This allows the computation code to be more consistent, which will make future optimizations easier to reason about because all paths now call into accumulateDataToTimings.
Samples used to be labelled with a SelectedState, which conflated two questions: how a sample should be drawn in the activity graph, and how the sample relates to the selected call node. Split those apart: - SampleRelationToNode describes the relation, and distinguishes samples whose stack *ends* at the node (TotalAndSelf) from samples which merely pass through it (TotalButNotSelf). This will let us share work between the activity graph and the sidebar, see the next commit. - FillBucket is the old SelectedState, renamed to what it actually is: the four fills the activity graph draws per category. It makes no distinction between a node's self samples and its non-self samples. SampleRelationToNode members are written as a FillBucket plus a "self" flag bit above it, which keeps SampleRelations.fillBucket a single mask rather than a table lookup, since it runs once per sample in the activity graph's loop. The per-sample array is now wrapped in a SampleRelations class instead of being passed around as a bare Uint8Array. A Uint8Array erases its element type, so indexing one yields a plain number; going through the accessors keeps the element type visible to the type checker and keeps the bit layout in one place. Consumers which only care about "filtered out" and "counts towards the total" use the isFilteredOut and contributesToTotal predicates. getSampleRelationsToNode takes the node as a parameter and is not named after the selection, even though every caller here passes the selected node: a later commit computes relations to the node hovered in the flame graph.
getTimingsForCallNodeIndex walked the stacks itself, with separate inverted and non-inverted code paths. Replace it with getCallNodeTimings, which consumes the SampleRelations that the activity graph already computes, and so needs neither code path: the relation already encodes whether a sample is filtered out, counts towards the needle's total, and counts towards its self time. Rename getSampleIndexToNonInvertedCallNodeIndexForFilteredThread to getSampleCallNodesForFilteredThread, and add getPreviewFilteredCtssSampleCallNodes / getPreviewFilteredCtssSampleRelations so that the sidebar timings no longer have to recompute the relations. The preview-filtered relations are a subarray of the filtered ones rather than a fresh computation. The flame graph tooltip computes relations to the *hovered* node, which is why getSampleRelationsToNode is not specific to the selection. The two profile-query call sites that computed a CallNodeSelfAndSummary now use getPreviewFilteredCtssSampleCallNodes rather than the filtered-thread array, since they index it with getPreviewFilteredCtssSamples. The two are only interchangeable while the summary strategy is 'timing' and there is no preview selection, which is true for every profile-query caller today, so this does not change any output.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6329 +/- ##
==========================================
+ Coverage 83.92% 83.94% +0.02%
==========================================
Files 353 354 +1
Lines 37969 38015 +46
Branches 10604 10730 +126
==========================================
+ Hits 31865 31912 +47
+ Misses 5675 5673 -2
- Partials 429 430 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Main | Deploy preview
Both the sidebar and the activity graph want to know, for each sample, whether the sample's stack belongs to the selected call node's subtree: The activity graph uses it to highlight graph areas, and the sidebar uses it to compute the "total" number.
This patch stack introduces a "sample relation to node" buffer and then uses it for both places.
Profile showing before + after: https://share.firefox.dev/3VBA03j
1090 vs 205 samples in the left flame graph branch, 5x faster!
(The right flame graph branch will be addressed by #5888.)