From 2fc0c23ea0c9e849f6408560c570586dfd14c5a1 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Thu, 1 Dec 2022 16:03:04 +0100 Subject: [PATCH 1/4] Procure a selection also when the components update (for example when changing threads) --- src/components/calltree/CallTree.js | 32 ++++++++++++++++++----------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index cdfbda64eb..7e52202b11 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -144,27 +144,29 @@ class CallTreeImpl extends PureComponent { componentDidMount() { this.focus(); - if (this.props.selectedCallNodeIndex === null) { - this.procureInterestingInitialSelection(); - } else if (this._treeView) { + this.maybeProcureInterestingInitialSelection(); + + if (this.props.selectedCallNodeIndex === null && this._treeView) { this._treeView.scrollSelectionIntoView(); } } componentDidUpdate(prevProps) { if ( - this.props.scrollToSelectionGeneration > - prevProps.scrollToSelectionGeneration + this.props.focusCallTreeGeneration > prevProps.focusCallTreeGeneration ) { - if (this._treeView) { - this._treeView.scrollSelectionIntoView(); - } + this.focus(); } + this.maybeProcureInterestingInitialSelection(); + if ( - this.props.focusCallTreeGeneration > prevProps.focusCallTreeGeneration + this.props.selectedCallNodeIndex !== null && + this.props.scrollToSelectionGeneration > + prevProps.scrollToSelectionGeneration && + this._treeView ) { - this.focus(); + this._treeView.scrollSelectionIntoView(); } } @@ -228,10 +230,16 @@ class CallTreeImpl extends PureComponent { openSourceView(file, 'calltree'); }; - procureInterestingInitialSelection() { + maybeProcureInterestingInitialSelection() { // Expand the heaviest callstack up to a certain depth and select the frame // at that depth. - const { tree, expandedCallNodeIndexes } = this.props; + const { tree, expandedCallNodeIndexes, selectedCallNodeIndex } = this.props; + + if (selectedCallNodeIndex !== null || expandedCallNodeIndexes.length > 0) { + // Let's not change some existing state. + return; + } + const newExpandedCallNodeIndexes = expandedCallNodeIndexes.slice(); const maxInterestingDepth = 17; // scientifically determined let currentCallNodeIndex = tree.getRoots()[0]; From 78ca8eb1a214cc8f7f159d2a56694c2f145b6b25 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 5 Dec 2022 14:13:06 +0100 Subject: [PATCH 2/4] Skip idle nodes when opening an initial selection. Also open up to 100 lines --- src/components/calltree/CallTree.js | 75 ++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 23 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index 7e52202b11..b453d4ce29 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -20,6 +20,7 @@ import { getScrollToSelectionGeneration, getFocusCallTreeGeneration, getPreviewSelection, + getCategories, } from 'firefox-profiler/selectors/profile'; import { selectedThreadSelectors } from 'firefox-profiler/selectors/per-thread'; import { @@ -37,6 +38,7 @@ import type { ImplementationFilter, ThreadsKey, CallNodeInfo, + CategoryList, IndexIntoCallNodeTable, CallNodeDisplayData, WeightType, @@ -54,6 +56,7 @@ type StateProps = {| +focusCallTreeGeneration: number, +tree: CallTreeType, +callNodeInfo: CallNodeInfo, + +categories: CategoryList, +selectedCallNodeIndex: IndexIntoCallNodeTable | null, +rightClickedCallNodeIndex: IndexIntoCallNodeTable | null, +expandedCallNodeIndexes: Array, @@ -233,38 +236,63 @@ class CallTreeImpl extends PureComponent { maybeProcureInterestingInitialSelection() { // Expand the heaviest callstack up to a certain depth and select the frame // at that depth. - const { tree, expandedCallNodeIndexes, selectedCallNodeIndex } = this.props; + const { + tree, + expandedCallNodeIndexes, + selectedCallNodeIndex, + callNodeInfo: { callNodeTable }, + categories, + } = this.props; if (selectedCallNodeIndex !== null || expandedCallNodeIndexes.length > 0) { // Let's not change some existing state. return; } - const newExpandedCallNodeIndexes = expandedCallNodeIndexes.slice(); - const maxInterestingDepth = 17; // scientifically determined - let currentCallNodeIndex = tree.getRoots()[0]; - if (currentCallNodeIndex === undefined) { - // This tree is empty. - return; - } - newExpandedCallNodeIndexes.push(currentCallNodeIndex); - for (let i = 0; i < maxInterestingDepth; i++) { - const children = tree.getChildren(currentCallNodeIndex); - if (children.length === 0) { + const newExpandedCallNodeIndexes = []; + // This value is completely arbitrary and looked good on Julien's machine + // when this was implemented. In the future we may want to look at the + // available space instead. + const maxVisibleLines = 70; + + const idleCategoryIndex = categories.findIndex( + (category) => category.name === 'Idle' + ); + + let children = tree.getRoots(); + let nodeToSelect = null; + let visibleLinesCount = children.length; + + while (true) { + const firstNonIdleNode = children.find( + (nodeIndex) => callNodeTable.category[nodeIndex] !== idleCategoryIndex + ); + + if (firstNonIdleNode === undefined) { + break; + } + + nodeToSelect = firstNonIdleNode; + + children = tree.getChildren(firstNonIdleNode); + + if (visibleLinesCount + children.length > maxVisibleLines) { + // Expanding this node would exceed our budget. break; } - currentCallNodeIndex = children[0]; - newExpandedCallNodeIndexes.push(currentCallNodeIndex); + + newExpandedCallNodeIndexes.push(firstNonIdleNode); + visibleLinesCount += children.length; } - this._onExpandedCallNodesChange(newExpandedCallNodeIndexes); - - const category = tree.getDisplayData(currentCallNodeIndex).categoryName; - if (category !== 'Idle') { - // If we selected the call node with a "idle" category, we'd have a - // completely dimmed activity graph because idle stacks are not drawn in - // this graph. Because this isn't probably what the average user wants we - // do it only when the category is something different. - this._onSelectedCallNodeChange(currentCallNodeIndex); + + if (newExpandedCallNodeIndexes.length > 0) { + // Take care to not trigger a state change if there's nothing to change, + // to avoid infinite render loop. + this._onExpandedCallNodesChange(newExpandedCallNodeIndexes); + } + + if (nodeToSelect !== null) { + this._onSelectedCallNodeChange(nodeToSelect); } } @@ -316,6 +344,7 @@ export const CallTree = explicitConnect<{||}, StateProps, DispatchProps>({ focusCallTreeGeneration: getFocusCallTreeGeneration(state), tree: selectedThreadSelectors.getCallTree(state), callNodeInfo: selectedThreadSelectors.getCallNodeInfo(state), + categories: getCategories(state), selectedCallNodeIndex: selectedThreadSelectors.getSelectedCallNodeIndex(state), rightClickedCallNodeIndex: From b63127cda44b4955f43818b2f1918203800b3ee9 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Tue, 6 Dec 2022 11:54:35 +0100 Subject: [PATCH 3/4] Always open more depths, and don't open uninteresting children --- src/components/calltree/CallTree.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index b453d4ce29..dd111d2f10 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -254,6 +254,7 @@ class CallTreeImpl extends PureComponent { // when this was implemented. In the future we may want to look at the // available space instead. const maxVisibleLines = 70; + const minimumDepth = 10; const idleCategoryIndex = categories.findIndex( (category) => category.name === 'Idle' @@ -274,9 +275,19 @@ class CallTreeImpl extends PureComponent { nodeToSelect = firstNonIdleNode; + const nodeData = tree.getNodeData(firstNonIdleNode); + if (nodeData.self > nodeData.total * 0.95) { + // This node doesn't have interesting children, let's stop here. + break; + } + + const depth = tree.getDepth(firstNonIdleNode); children = tree.getChildren(firstNonIdleNode); - if (visibleLinesCount + children.length > maxVisibleLines) { + if ( + depth > minimumDepth && + visibleLinesCount + children.length > maxVisibleLines + ) { // Expanding this node would exceed our budget. break; } From f81ef65647e19d097f6c6a2a1e737671fe7a4364 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 5 Dec 2022 17:21:59 +0100 Subject: [PATCH 4/4] Update tests --- .../components/ProfileCallTreeView.test.js | 12 +- .../ProfileCallTreeView.test.js.snap | 324 +----------------- 2 files changed, 15 insertions(+), 321 deletions(-) diff --git a/src/test/components/ProfileCallTreeView.test.js b/src/test/components/ProfileCallTreeView.test.js index 9e9ed6cf3a..1c547c0fc8 100644 --- a/src/test/components/ProfileCallTreeView.test.js +++ b/src/test/components/ProfileCallTreeView.test.js @@ -283,14 +283,14 @@ describe('calltree/ProfileCallTreeView', function () { expect(getRowElement('C')).toHaveClass('isSelected'); }); - it('does not select the heaviest stack if it is idle', () => { + it('does select a non-idle stack if the heaviest stack is idle', () => { const { profile } = getProfileFromTextSamples(` - A A A A A - B C[cat:Idle] C[cat:Idle] C[cat:Idle] D - E E + A A A A A A + B C[cat:Idle] C[cat:Idle] C[cat:Idle] B D + E E F `); - const { container } = setup(profile); - expect(container.querySelector('.treeViewRow.isSelected')).toBeFalsy(); + const { getRowElement } = setup(profile); + expect(getRowElement('E')).toHaveClass('isSelected'); }); }); diff --git a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap index 53f3829e06..151520cc41 100644 --- a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap +++ b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap @@ -3795,7 +3795,7 @@ for understanding where time was actually spent in a program." class="react-contextmenu-wrapper treeViewContextMenu" >
- - 67% - - - 2 - - - 2 - - -
- -
-
- — - - -
- -
-
- - 67% - - 2 - - — - - - 33% - - - 1 - - - — - - -
- -
-
- - 33% - - - 1 - - - — - - -
- -
-
- - 33% - - - 1 - - - — - - -
- -
-
- - - - - Z - - -
-
- - - - - Y - - -
-
- - - - - X - - - libX.so - -
-
- - - - - B - - -
-
- -
-