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:
|
canova
requested changes
Sep 21, 2026
canova
left a comment
canova
left a comment
Member
There was a problem hiding this comment.
Thanks, it's super cool!
One thing I noticed is that the more we hover over nodes in the flame graph, the higher the memory usage goes due to the memoize that we are adding. Is it possible to improve that?
| const callNodeInfoInverted = callNodeInfo.asInverted(); | ||
| const isInvertedRoot = | ||
| callNodeInfoInverted !== null && | ||
| callNodeInfoInverted.isRoot(callNodeIndex); |
Member
There was a problem hiding this comment.
Nit: Can we also use isRoot in _getSampleRelationsInverted where we do depthForNode() === 0?
| // the category breakdown. sampleRelations tells us whether each sample is | ||
| // filtered out, counts towards the needle's total time, and whether it also | ||
| // counts towards its self time. This works for both inverted and | ||
| // non-inverted call trees, because getSampleRelationsToSelectedNode handles |
Member
There was a problem hiding this comment.
Nit: getSampleRelationsToSelectedNode doesn't exist currently, I think it should be getSampleRelationsToNode.
Comment on lines
+360
to
+362
| _getSampleRelations = memoize(getSampleRelationsToNode, { | ||
| cache: new MixedTupleMap(), | ||
| }); |
Member
There was a problem hiding this comment.
Does this mean that we start caching for every hovered flame graph node? I think this might get large quickly.
I did some manual testing where I opened a flame graph in the deploy preview in this PR, and then started just hovering a bunch of nodes. After ~25seconds each I see +430MB vs +1.46GB.
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
2 participants
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.)