Skip to content

[deploy-preview] Butterfly view - #2388

Open
julienw wants to merge 5 commits into
firefox-devtools:mainfrom
julienw:butterfly-view-2
Open

[deploy-preview] Butterfly view#2388
julienw wants to merge 5 commits into
firefox-devtools:mainfrom
julienw:butterfly-view-2

Conversation

@julienw

@julienw julienw commented Feb 6, 2020

Copy link
Copy Markdown
Contributor

This is a rebase of #1538:

  • removed the "360° view"
  • rewrote to show running times instead of self times.

The goal is to start a discussion about the usefulness of this panel.

deploy preview

What needs to be done to finish this:

  • add the "settings" bar to toggle JS mode
  • scroll when iterating with the keyboard
  • what happens when selecting a function?
  • action on a double click focus on a function (maybe?)
  • decide on the layout
  • some CSS to make it nicer
  • fix the tests and add new ones covering the new components

┆Issue is synchronized with this Jira Task

@julienw
julienw force-pushed the butterfly-view-2 branch 2 times, most recently from 41b1d9b to 75873ca Compare February 10, 2020 13:25
Comment on lines +14 to +16
type State = {|
selectedFuncIndex: IndexIntoFuncTable | null,
|};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This may be stored in a redux state later, so that other parts of the app can also react to this selection.

@mstange mstange 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.

I'm really happy that you're working on this! I think this panel will be an important addition.

I have a few notes on the implementation. First of all, I think we need to pay attention to the performance during preview range selection. At the moment, dragging a selection is really janky.
Your current algorithm operates on the preview filtered thread. It starts with the samples, then computes some sample times, and then iterates over the stacks and does two things at the same time: building up a cache of "does stack N contain func M" (represented as an array of sets) and accumulating func times. I think it would be better to do it the other way around: Independent of the samples, compute the "does stack N contain func M" cache so that it can be memoized and reused for different preview selections. And then have a selector that combines the preview selection filtered samples with the cache and returns the times per function for the current selection.
For reference, it can be useful to compare to what the call tree is doing. It's calculating the CallNodeInfo independent of the preview selection, and then it combines the sample information with the CallNodeInfo later (computeCallTreeCountsAndTimings).

Also, we should embrace the fact that a global "invert call tree" boolean is not meaningful in this new panel. There should probably be new selectors that give you the filtered-but-never-inverted thread, and the preview-filtered-but-never-inverted thread, so that you can make use of those when computing the functions list.

@mstange

mstange commented Feb 10, 2020

Copy link
Copy Markdown
Contributor

Furthermore, rather than showing just the immediate callers and callees, we should show two call trees: The callee tree which is the same as the "call tree focused on function" and the callers tree which is the same as the "inverted call tree focused on callers of function" (which is something that we don't have yet).

@gregtatum gregtatum closed this Jun 30, 2020
@gregtatum gregtatum reopened this Jun 30, 2020
@gregtatum
gregtatum changed the base branch from master to main June 30, 2020 19:27
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.

3 participants