Make it easier to create upside-down flame graphs#6128
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6128 +/- ##
==========================================
- Coverage 83.47% 83.46% -0.02%
==========================================
Files 342 342
Lines 36338 36346 +8
Branches 10177 10208 +31
==========================================
+ Hits 30334 30335 +1
- Misses 5576 5583 +7
Partials 428 428 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mstange
force-pushed
the
push-tzmnxpxrpyln
branch
from
1d18f2a to
161003a
Compare
|
I was in the middle of reviewing this 😃 Is it safe to review now? |
Oops sorry! Yes it is safe to review, I haven't made any changes, this was just a rebase. |
| : Math.ceil(viewportBottom / stackFrameHeight); | ||
|
|
||
| // Only draw the stack frames that are vertically within view. | ||
| // The graph is drawn from bottom to top, in order of increasing depth. |
There was a problem hiding this comment.
The comment is misleading now, need to reword "drawn from bottom to top". Drawn from either bottom to top or top to bottom?
There was a problem hiding this comment.
Changed to:
// Rows are iterated in order of increasing depth; in startsAtBottom
// mode that means visually bottom to top, otherwise top to bottom.
| const cssRowTop: CssPixels = startsAtBottom | ||
| ? (maxStackDepthPlusOne - depth - 1) * ROW_HEIGHT - viewportTop | ||
| : depth * ROW_HEIGHT - viewportTop; | ||
| const cssRowBottom: CssPixels = startsAtBottom |
There was a problem hiding this comment.
Can be replaced by simply cssRowTop + ROW_HEIGHT.
| // is maintained. | ||
| if (prevProps.maxStackDepthPlusOne !== this.props.maxStackDepthPlusOne) { | ||
| // is maintained. In top-down layout the base is at the top, so no | ||
| // adjustment is needed when depth grows or shrinks. |
There was a problem hiding this comment.
This does not seem to be true regarding shrink: if the user had scrolled down so viewportBottom > newMaxViewportHeight, nothing clamps it, so blank space is shown below the now-shorter graph until the next scroll/resize event fires a moveViewport.
There was a problem hiding this comment.
Oh, good find! Should be fixed now.
… flame graph code. For the non-inverted call node info, the two are equivalent. But if we want to use the flame graph on an inverted call node info, such as for the "called by" view in the function list, the old code would not work because the callNodeTable is always the non-inverted call node table. So this change removes one barrier for inverted flame graphs.
startsAtBottom={true} is the regular flame graph layout.
startsAtBottom={false} can be used for icicle-style flame graphs.
mstange
force-pushed
the
push-tzmnxpxrpyln
branch
from
161003a to
c765a85
Compare
2 participants
This PR extracts some ideas from #6090. But it doesn't add UI to show an inverted flame graph.
I'm planning to have an upside-down flame graph in the function list (for the callers of the selected function), so the function list PR depends on this one.