From 2fc0c23ea0c9e849f6408560c570586dfd14c5a1 Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Thu, 1 Dec 2022 16:03:04 +0100 Subject: [PATCH 1/5] 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 2c62a417815fa752ff22af95731e467b34eead2d Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 5 Dec 2022 14:13:06 +0100 Subject: [PATCH 2/5] 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 f10a08469ad59f828c32b88ca7ed31867408e55c Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 5 Dec 2022 16:37:16 +0100 Subject: [PATCH 3/5] Also open interesting nodes besides the first one --- src/components/calltree/CallTree.js | 33 ++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index 0188b9cab1..66f402e7bd 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -250,10 +250,12 @@ class CallTreeImpl extends PureComponent { } const newExpandedCallNodeIndexes = []; + let nodeToSelect = null; + // 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' @@ -261,9 +263,8 @@ class CallTreeImpl extends PureComponent { let nodesToVisit = tree.getRoots(); 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,28 +275,40 @@ class CallTreeImpl extends PureComponent { (sum, nodeIndex) => sum + tree.getNodeData(nodeIndex).total, 0 ); - - nodeToSelect = nonIdleNodes[0]; + const runningTimeThreshold = sumOfNonIdleRunningTime / 20; for (let i = 0; i < nonIdleNodes.length; i++) { const nodeIndex = nonIdleNodes[i]; - if (i > 0) { - // TODO + if (i === 0) { + nodeToSelect = nodeIndex; + } else { // 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. + // The remaining nodes will have even less, let's break out of the + // inner loop. + break; + } } 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); + + if (i === 0) { + // Look at the children of the heaviest node only. + newNodesToVisit.push(...children); + } } nodesToVisit = newNodesToVisit; From abdb877e36e050bb92761a0faca82f034187747d Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Tue, 6 Dec 2022 11:41:01 +0100 Subject: [PATCH 4/5] Always open more depths, and don't open uninteresting children --- src/components/calltree/CallTree.js | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index 66f402e7bd..eb9f67e51d 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -256,6 +256,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' @@ -279,13 +280,21 @@ class CallTreeImpl extends PureComponent { for (let i = 0; i < nonIdleNodes.length; i++) { const nodeIndex = nonIdleNodes[i]; + const nodeData = tree.getNodeData(nodeIndex); if (i === 0) { nodeToSelect = nodeIndex; - } else { + } + + 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. // The remaining nodes will have even less, let's break out of the @@ -295,8 +304,13 @@ class CallTreeImpl extends PureComponent { } const children = tree.getChildren(nodeIndex); + const nodeDepth = tree.getDepth(nodeIndex); - if (visibleLinesCount + children.length > maxVisibleLines) { + if ( + i !== 0 && + nodeDepth > minimumDepth && + 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 f023e511da4cae05abf651ec2629a70096e55b1e Mon Sep 17 00:00:00 2001 From: Julien Wajsberg Date: Mon, 5 Dec 2022 17:21:59 +0100 Subject: [PATCH 5/5] Update tests --- .../components/ProfileCallTreeView.test.js | 12 +- .../ProfileCallTreeView.test.js.snap | 1461 +++++++++++------ 2 files changed, 1010 insertions(+), 463 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..874036df32 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 + + + — + + +
+ +
+
@@ -863,7 +924,7 @@ allocated or deallocated in the program." >
+
+ + -20% + + + -3 + + + — + + +
+ +
+
@@ -1534,7 +1656,7 @@ allocated or deallocated in the program." >
+
+ + 27% + + + 11 + + + — + + +
+ +
+
@@ -2193,7 +2376,7 @@ allocated or deallocated in the program." >
+
+ + 20% + + + 3 + + + — + + +
+ +
-
- - - - - - -`; - -exports[`ProfileCallTreeView with unbalanced native allocations matches the snapshot for native deallocations 1`] = ` -
-
-
+
+ + + + + + +`; + +exports[`ProfileCallTreeView with unbalanced native allocations matches the snapshot for native deallocations 1`] = ` +
+
+
  • + + -59% + + + -24 + + + -11 + + +
    + +
    +
    +
    - 67% + 33% - 2 + 1 - — + 1
    +
    +
    +
    +
    +