firefox-devtools / profiler

Firefox Profiler — Web app for Firefox performance analysis
https://profiler.firefox.com
Mozilla Public License 2.0
1.19k stars 390 forks source link

Clicking a tracing marker in the header should focus a heavy callstack within that range #54

Open mstange opened 7 years ago

mstange commented 7 years ago

┆Issue is synchronized with this Jira Task

gregtatum commented 7 years ago

What is a "heavy callstack"?

mstange commented 7 years ago

The "heaviest" callstack is the stack with the highest number of samples in it. Not sure why I said "heavy" instead of "heaviest" when I filed the bug.

My current thinking is that we should find the heaviest callstack, and then select the leafmost frame in it that has a different category from its parent frame.

gregtatum commented 6 years ago

I'm not sure I would actually want this if I already have something I've already selected. Do you want to keep this open @mstange ?

mstange commented 6 years ago

Yes, I still think this would be a good change.

gregtatum commented 6 years ago

This is a screenshot of the markers being referenced in this bug:

image

And here is generally how the code should start to look for this patch:

diff --git a/src/actions/profile-view.js b/src/actions/profile-view.js
index b1e6a745..eb19b2d5 100644
--- a/src/actions/profile-view.js
+++ b/src/actions/profile-view.js
@@ -212,6 +212,16 @@ export function updateProfileSelection(selection: ProfileSelection): Action {
   };
 }

+export function selectHeaviestCallNodeInSelection(): ThunkAction {
+  return (dispatch, getState) => {
+    // Get the call tree, and find the heaviest sample.
+    // Then computed the expandedCallNodePath and selectedCallNode
+
+    dispatch(changeSelectedCallNode( ... ));
+    dispatch(changeExpandedCallNodes( ... ));
+  };
+}
+
 export function addRangeFilter(start: number, end: number): Action {
   return {
     type: 'ADD_RANGE_FILTER',
diff --git a/src/components/header/ProfileThreadHeaderBar.js b/src/components/header/ProfileThreadHeaderBar.js
index c2c698e7..08fdc56b 100644
--- a/src/components/header/ProfileThreadHeaderBar.js
+++ b/src/components/header/ProfileThreadHeaderBar.js
@@ -27,7 +27,10 @@ import type {
 } from '../../types/profile-derived';
 import type { State } from '../../types/reducers';

-import { updateProfileSelection } from '../../actions/profile-view';
+import {
+  updateProfileSelection,
+  selectHeaviestCallNodeInSelection,
+} from '../../actions/profile-view';

 type Props = {
   threadIndex: ThreadIndex,
@@ -45,6 +48,7 @@ type Props = {
   processDetails: string,
   changeSelectedThread: ThreadIndex => void,
   updateProfileSelection: typeof updateProfileSelection,
+  selectHeaviestCallNodeInSelection: typeof selectHeaviestCallNodeInSelection,
   changeSelectedCallNode: (IndexIntoCallNodeTable, CallNodePath) => void,
 };

@@ -106,14 +110,16 @@ class ProfileThreadHeaderBar extends PureComponent<Props> {
       rangeEnd,
       updateProfileSelection,
       changeSelectedThread,
+      selectHeaviestCallNodeInSelection,
     } = this.props;
+    changeSelectedThread(threadIndex);
     updateProfileSelection({
       hasSelection: true,
       isModifying: false,
       selectionStart: Math.max(rangeStart, start),
       selectionEnd: Math.min(rangeEnd, end),
     });
-    changeSelectedThread(threadIndex);
+    selectHeaviestCallNodeInSelection();
   }

   _onMarkerSelect(/* markerIndex */) {}
gregtatum commented 6 years ago

@mstange To clarify what you're saying, would the heaviest stack be the node in the call tree with the largest self time?

mstange commented 6 years ago

It should do something very similar to what this does:

https://github.com/devtools-html/perf.html/blob/7d9ac63cb58629a458d96a7a9150716cdc358ee4/src/components/calltree/CallTree.js#L152-L170

That is, it should expand the path "along the top" of the tree up to a certain depth. That's usually the part of the tree that I'm interested in when I click a marker.

julienw commented 5 years ago

Removing "help wanted" because this seems a bit complicated for contributors. Not sure how to move that forward though...