From 2fc0c23ea0c9e849f6408560c570586dfd14c5a1 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Thu, 1 Dec 2022 16:03:04 +0100 Subject: [PATCH 1/6] 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 9e7e46d94e6afeab863f06f3eb8ddfd018e220e5 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 5 Dec 2022 14:13:06 +0100 Subject: [PATCH 2/6] Skip idle nodes when opening an initial selection. Also open up to 100 lines --- src/components/calltree/CallTree.js | 93 +++++++++++++++++++++-------- 1 file changed, 69 insertions(+), 24 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index 7e52202b11..0188b9cab1 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,79 @@ 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) { - break; + 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 = 100; + + const idleCategoryIndex = categories.findIndex( + (category) => category.name === 'Idle' + ); + + let nodesToVisit = tree.getRoots(); + let visibleLinesCount = nodesToVisit.length; + let nodeToSelect = null; + + outer: while (nodesToVisit.length) { + const newNodesToVisit = []; + const nonIdleNodes = nodesToVisit.filter( + (nodeIndex) => callNodeTable.category[nodeIndex] !== idleCategoryIndex + ); + + // Look at the sum of the running times for all non-idle nodes + const sumOfNonIdleRunningTime = nonIdleNodes.reduce( + (sum, nodeIndex) => sum + tree.getNodeData(nodeIndex).total, + 0 + ); + + nodeToSelect = nonIdleNodes[0]; + + for (let i = 0; i < nonIdleNodes.length; i++) { + const nodeIndex = nonIdleNodes[i]; + + if (i > 0) { + // TODO + // The first node is always expanded, but let's check whether we + // should open more nodes at this level. + break; + } + + const children = tree.getChildren(nodeIndex); + + if (visibleLinesCount + children.length > maxVisibleLines) { + break outer; + } + + newExpandedCallNodeIndexes.push(nodeIndex); + visibleLinesCount += children.length; + newNodesToVisit.push(...children); } - currentCallNodeIndex = children[0]; - newExpandedCallNodeIndexes.push(currentCallNodeIndex); + + nodesToVisit = newNodesToVisit; } - 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 +360,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 c8b99c8e92e9ccca3d48faeb59a11f8f051b259a Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 5 Dec 2022 16:37:16 +0100 Subject: [PATCH 3/6] Also open interesting nodes besides the first one --- src/components/calltree/CallTree.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index 0188b9cab1..61eee0ab82 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -253,7 +253,7 @@ class CallTreeImpl extends PureComponent { // 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 = 100; + const maxVisibleLines = 70; const idleCategoryIndex = categories.findIndex( (category) => category.name === 'Idle' @@ -263,7 +263,7 @@ class CallTreeImpl extends PureComponent { let visibleLinesCount = nodesToVisit.length; let nodeToSelect = null; - outer: while (nodesToVisit.length) { + while (nodesToVisit.length) { const newNodesToVisit = []; const nonIdleNodes = nodesToVisit.filter( (nodeIndex) => callNodeTable.category[nodeIndex] !== idleCategoryIndex @@ -274,6 +274,7 @@ class CallTreeImpl extends PureComponent { (sum, nodeIndex) => sum + tree.getNodeData(nodeIndex).total, 0 ); + const runningTimeThreshold = sumOfNonIdleRunningTime / 20; nodeToSelect = nonIdleNodes[0]; @@ -281,21 +282,27 @@ class CallTreeImpl extends PureComponent { const nodeIndex = nonIdleNodes[i]; if (i > 0) { - // TODO // The first node is always expanded, but let's check whether we // should open more nodes at this level. - break; + const thisRunningTime = tree.getNodeData(nodeIndex).total; + if (thisRunningTime < runningTimeThreshold) { + // This node doesn't have a lot of running time compared to all the + // others we consider, let's move to the next one. + continue; + } } const children = tree.getChildren(nodeIndex); if (visibleLinesCount + children.length > maxVisibleLines) { - break outer; + // Expanding this node would exceed our budget. + // Let's look at the other nodes in case they have a smaller amount of children. + continue; } newExpandedCallNodeIndexes.push(nodeIndex); - visibleLinesCount += children.length; newNodesToVisit.push(...children); + visibleLinesCount += children.length; } nodesToVisit = newNodesToVisit; From e2fe1a3c8bbb5acd4c4b2f92af06b698792ade28 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 5 Dec 2022 18:36:09 +0100 Subject: [PATCH 4/6] Only select a node in the first-child inheritance line, excluding the idle nodes --- src/components/calltree/CallTree.js | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index 61eee0ab82..80a1872a11 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -261,7 +261,6 @@ class CallTreeImpl extends PureComponent { let nodesToVisit = tree.getRoots(); let visibleLinesCount = nodesToVisit.length; - let nodeToSelect = null; while (nodesToVisit.length) { const newNodesToVisit = []; @@ -276,8 +275,6 @@ class CallTreeImpl extends PureComponent { ); const runningTimeThreshold = sumOfNonIdleRunningTime / 20; - nodeToSelect = nonIdleNodes[0]; - for (let i = 0; i < nonIdleNodes.length; i++) { const nodeIndex = nonIdleNodes[i]; @@ -314,6 +311,31 @@ class CallTreeImpl extends PureComponent { this._onExpandedCallNodesChange(newExpandedCallNodeIndexes); } + // Now we want to select the deepest node when following the first + // non-idle children only. + let nodeToSelect = null; + let children = tree.getRoots(); + while (true) { + const firstNonIdleChild = children.find( + (nodeIndex) => callNodeTable.category[nodeIndex] !== idleCategoryIndex + ); + if (firstNonIdleChild === undefined) { + // No suitable child has been found, let's break out of the loop. + break; + } + + // This is a good candidate, until the next iteration at least. + nodeToSelect = firstNonIdleChild; + if (!newExpandedCallNodeIndexes.includes(nodeToSelect)) { + // This is the first node that wasn't expanded in the previous loop, + // then let's stop right here. + break; + } + + // Otherwise, let's go deeper. + children = tree.getChildren(firstNonIdleChild); + } + if (nodeToSelect !== null) { this._onSelectedCallNodeChange(nodeToSelect); } From bb9f9a25eae09089d343d905d8fe173e7eab9a83 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Tue, 6 Dec 2022 11:58:41 +0100 Subject: [PATCH 5/6] Always open more depths, and don't open uninteresting children --- src/components/calltree/CallTree.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index 80a1872a11..7060cc96f8 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 minimalDepth = 10; const idleCategoryIndex = categories.findIndex( (category) => category.name === 'Idle' @@ -277,11 +278,17 @@ class CallTreeImpl extends PureComponent { for (let i = 0; i < nonIdleNodes.length; i++) { const nodeIndex = nonIdleNodes[i]; + const nodeData = tree.getNodeData(nodeIndex); + + if (nodeData.self > nodeData.total * 0.95) { + // This node doesn't have interesting children, let's move on. + continue; + } if (i > 0) { // The first node is always expanded, but let's check whether we // should open more nodes at this level. - const thisRunningTime = tree.getNodeData(nodeIndex).total; + const thisRunningTime = nodeData.total; if (thisRunningTime < runningTimeThreshold) { // This node doesn't have a lot of running time compared to all the // others we consider, let's move to the next one. @@ -290,8 +297,13 @@ class CallTreeImpl extends PureComponent { } const children = tree.getChildren(nodeIndex); + const depth = tree.getDepth(nodeIndex); - if (visibleLinesCount + children.length > maxVisibleLines) { + if ( + i !== 0 && + depth > minimalDepth && + visibleLinesCount + children.length > maxVisibleLines + ) { // Expanding this node would exceed our budget. // Let's look at the other nodes in case they have a smaller amount of children. continue; From a834cbeeba489b9f5283167617728a0ccfcb665b Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 5 Dec 2022 17:21:59 +0100 Subject: [PATCH 6/6] Update tests --- .../components/ProfileCallTreeView.test.js | 12 +- .../ProfileCallTreeView.test.js.snap | 1729 ++++++++++++----- 2 files changed, 1294 insertions(+), 447 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..c96ead4bb9 100644 --- a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap +++ b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap @@ -192,7 +192,7 @@ allocated or deallocated in the program." >
+
+ + 20% + + + 3 + + + — + + +
+ +
+
+ + 20% + + + 3 + + + 3 + + +
+ +
+
+ + + + + D + + +
+
+ +
@@ -863,7 +984,7 @@ allocated or deallocated in the program." >
+
+ + -20% + + + -3 + + + — + + +
+ +
+
+ + -20% + + + -3 + + + -3 + + +
+ +
+
+ + + + + D + + +
+
+ +
@@ -1534,7 +1776,7 @@ allocated or deallocated in the program." >
-
- -
+ + 27% + + + 11 + + + — + + +
+ +
+
+ + 27% + + + 11 + + + 11 + + +
+ +
+
+
+
+
+ + + + + D + + +
+
+ +
@@ -2193,7 +2556,7 @@ allocated or deallocated in the program." >
+
+ + 20% + + + 3 + + + — + + +
+ +
+
+ + 20% + + + 3 + + + 3 + + +
+ +
+
+ + + + + D + + +
+
+ +
@@ -2852,7 +3336,7 @@ allocated or deallocated in the program." >
+ -59% + + + -24 + + + -11 + + +
+ +
+
+ + -32% + + + -13 + + + -13 + + +
+ +
+
+ -41% @@ -3020,7 +3566,7 @@ allocated or deallocated in the program."
+
+ + + + + J + + +
+
+ +
- 67% + 33% - 2 + 1 - — + 1
+
+
+
+
+