Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mstange opened this issue Nov 30, 2016 · 8 comments
Labels
call tree Related to the call tree panel polish Small features or changes that do not require planning to work on. These help out our end users.

Comments

@mstange
Copy link
Contributor

mstange commented Nov 30, 2016

┆Issue is synchronized with this Jira Task

@gregtatum
Copy link
Member

What is a "heavy callstack"?

@gregtatum gregtatum changed the title Clicking a timeline tracing marker at the top should focus a heavy callstack within that range Clicking a tracing marker in the header should focus a heavy callstack within that range Mar 8, 2017
@gregtatum gregtatum added the feature Work that is user facing, and typically should be planned through https://airtable.com/shrRydo6UXheb label Mar 8, 2017
@mstange
Copy link
Contributor Author

mstange commented Mar 8, 2017

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 gregtatum added the call tree Related to the call tree panel label Nov 28, 2017
@gregtatum
Copy link
Member

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
Copy link
Contributor Author

mstange commented Nov 29, 2017

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

@gregtatum gregtatum added polish Small features or changes that do not require planning to work on. These help out our end users. help wanted Things ready to be worked on by anyone. Issues must include instructions on how to complete the task and removed feature Work that is user facing, and typically should be planned through https://airtable.com/shrRydo6UXheb labels Nov 29, 2017
@gregtatum
Copy link
Member

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
Copy link
Member

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

@mstange
Copy link
Contributor Author

mstange commented Nov 29, 2017

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.

@gregtatum gregtatum added this to Backlog in Sprint Planning 2018 Jan 11, 2018
@gregtatum gregtatum moved this from Backlog to To Do in Sprint Planning 2018 Jan 12, 2018
@julienw julienw removed the help wanted Things ready to be worked on by anyone. Issues must include instructions on how to complete the task label Mar 7, 2019
@julienw
Copy link
Contributor

julienw commented Mar 7, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
call tree Related to the call tree panel polish Small features or changes that do not require planning to work on. These help out our end users.
Projects
No open projects
Development

No branches or pull requests

3 participants