From 795f0752957c2564a87fbeef03cc17e6f84a45c1 Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Sat, 21 Oct 2023 22:26:12 -0400 Subject: [PATCH 1/7] Compute new selection earlier when inverting. Compute the inverted selected call node in the action creator, not in the reducer. This means we don't have to pass the call tree as far down. Also move the computation into the CallTree class. And fix it to stop at nodes with heaviest self time, even if they're not leaf nodes. --- src/actions/profile-view.js | 10 +++++-- src/profile-logic/call-tree.js | 53 +++++++++++++++++++++++++++++++++ src/profile-logic/transforms.js | 49 ------------------------------ src/reducers/profile-view.js | 17 +++-------- src/test/store/actions.test.js | 12 ++++---- src/types/actions.js | 4 +-- 6 files changed, 71 insertions(+), 74 deletions(-) diff --git a/src/actions/profile-view.js b/src/actions/profile-view.js index 0367d88af7..19c0f10155 100644 --- a/src/actions/profile-view.js +++ b/src/actions/profile-view.js @@ -1772,13 +1772,17 @@ export function changeInvertCallstack( eventCategory: 'profile', eventAction: 'change invert callstack', }); + const callTree = selectedThreadSelectors.getCallTree(getState()); + const selectedCallNode = selectedThreadSelectors.getSelectedCallNodeIndex( + getState() + ); + const newSelectedCallNodePath = + callTree.findHeavyPathToSameFunctionAfterInversion(selectedCallNode); dispatch({ type: 'CHANGE_INVERT_CALLSTACK', invertCallstack, selectedThreadIndexes: getSelectedThreadIndexes(getState()), - callTree: selectedThreadSelectors.getCallTree(getState()), - callNodeTable: selectedThreadSelectors.getCallNodeInfo(getState()) - .callNodeTable, + newSelectedCallNodePath, }); }; } diff --git a/src/profile-logic/call-tree.js b/src/profile-logic/call-tree.js index 5f1499784b..e2d02f2d0d 100644 --- a/src/profile-logic/call-tree.js +++ b/src/profile-logic/call-tree.js @@ -20,6 +20,7 @@ import type { SamplesLikeTable, WeightType, CallNodeTable, + CallNodePath, IndexIntoCallNodeTable, CallNodeInfo, CallNodeData, @@ -362,6 +363,58 @@ export class CallTree { this._thread ); } + + /** + * Take a CallNodeIndex, and compute an inverted path for it. + * + * e.g: + * (invertedPath, invertedCallTree) => path + * (path, callTree) => invertedPath + * + * Call trees are sorted with the CallNodes with the heaviest total time as the first + * entry. This function walks to the tip of the heaviest branches to find the leaf node, + * then construct an inverted CallNodePath with the result. This gives a pretty decent + * result, but it doesn't guarantee that it will select the heaviest CallNodePath for the + * INVERTED call tree. This would require doing a round trip through the reducers or + * some other mechanism in order to first calculate the next inverted call tree. This is + * probably not worth it, so go ahead and use the uninverted call tree, as it's probably + * good enough. + */ + findHeavyPathToSameFunctionAfterInversion( + callNodeIndex: IndexIntoCallNodeTable | null + ): CallNodePath { + if (callNodeIndex === null) { + return []; + } + let parentSelf = 0; + let children = [callNodeIndex]; + const pathToLeaf = []; + do { + // Walk down the tree's depth to construct a path to the leaf node, this should + // be the heaviest branch of the tree. + const firstChild = children[0]; + const { total, self } = this.getNodeData(firstChild); + if (total < parentSelf) { + // The parent node was the node with the highest self time. We don't need + // to descend any deeper because we won't find another node with higher + // self time in this subtree because this subtree's total is already lower. + // ... except if we have negative weights, i.e. a diff profile, but we + // don't bother to handle those separately. + break; + } + parentSelf = self; + pathToLeaf.push(firstChild); + children = this.getChildren(firstChild); + } while (children && children.length > 0); + + return ( + pathToLeaf + // Map the CallNodeIndex to FuncIndex. + .map((index) => this._callNodeTable.func[index]) + // Reverse it so that it's in the proper inverted order. + .reverse() + ); + } } function _getInvertedStackSelf( diff --git a/src/profile-logic/transforms.js b/src/profile-logic/transforms.js index 3934daba2f..288ec897d2 100644 --- a/src/profile-logic/transforms.js +++ b/src/profile-logic/transforms.js @@ -9,7 +9,6 @@ import { } from '../utils/uintarray-encoding'; import { toValidImplementationFilter, - getCallNodeIndexFromPath, updateThreadStacks, updateThreadStacksByGeneratingNewStackColumns, getMapStackUpdater, @@ -18,7 +17,6 @@ import { import { timeCode } from '../utils/time-code'; import { assertExhaustiveCheck, convertToTransformType } from '../utils/flow'; import { canonicalizeRangeSet } from '../utils/range-set'; -import { CallTree } from '../profile-logic/call-tree'; import { getSearchFilteredMarkerIndexes } from '../profile-logic/marker-data'; import { shallowCloneFrameTable, getEmptyStackTable } from './data-structures'; import { getFunctionName } from './function-info'; @@ -699,53 +697,6 @@ function _callNodePathHasPrefixPath( ); } -/** - * Take a CallNodePath, and invert it given a CallTree. Note that if the CallTree - * is itself inverted, you will get back the uninverted CallNodePath to the regular - * CallTree. - * - * e.g: - * (invertedPath, invertedCallTree) => path - * (path, callTree) => invertedPath - * - * Call trees are sorted with the CallNodes with the heaviest total time as the first - * entry. This function walks to the tip of the heaviest branches to find the leaf node, - * then construct an inverted CallNodePath with the result. This gives a pretty decent - * result, but it doesn't guarantee that it will select the heaviest CallNodePath for the - * INVERTED call tree. This would require doing a round trip through the reducers or - * some other mechanism in order to first calculate the next inverted call tree. This is - * probably not worth it, so go ahead and use the uninverted call tree, as it's probably - * good enough. - */ -export function invertCallNodePath( - path: CallNodePath, - callTree: CallTree, - callNodeTable: CallNodeTable -): CallNodePath { - let callNodeIndex = getCallNodeIndexFromPath(path, callNodeTable); - if (callNodeIndex === null) { - // No path was found, return an empty CallNodePath. - return []; - } - let children = [callNodeIndex]; - const pathToLeaf = []; - do { - // Walk down the tree's depth to construct a path to the leaf node, this should - // be the heaviest branch of the tree. - callNodeIndex = children[0]; - pathToLeaf.push(callNodeIndex); - children = callTree.getChildren(callNodeIndex); - } while (children && children.length > 0); - - return ( - pathToLeaf - // Map the CallNodeIndex to FuncIndex. - .map((index) => callNodeTable.func[index]) - // Reverse it so that it's in the proper inverted order. - .reverse() - ); -} - /** * Transform a thread's stacks to merge stacks that match the CallNodePath into * the calling stack. See `src/types/transforms.js` for more information about the diff --git a/src/reducers/profile-view.js b/src/reducers/profile-view.js index d0e205f0b0..fbe0262efd 100644 --- a/src/reducers/profile-view.js +++ b/src/reducers/profile-view.js @@ -266,7 +266,7 @@ const viewOptionsPerThread: Reducer = ( }); } case 'CHANGE_INVERT_CALLSTACK': { - const { callTree, callNodeTable, selectedThreadIndexes } = action; + const { newSelectedCallNodePath, selectedThreadIndexes } = action; return objectMap(state, (viewOptions, threadsKey) => { if ( // `Object.entries` converts number threadsKeys into strings, so @@ -274,23 +274,14 @@ const viewOptionsPerThread: Reducer = ( threadsKey === ProfileData.getThreadsKey(selectedThreadIndexes).toString() ) { - // Only attempt this on the current thread, as we need the transformed thread - // There is no guarantee that this has been calculated on all the other threads, - // and we shouldn't attempt to expect it, as that could be quite a perf cost. - const selectedCallNodePath = Transforms.invertCallNodePath( - viewOptions.selectedCallNodePath, - callTree, - callNodeTable - ); - const expandedCallNodePaths = new PathSet(); - for (let i = 1; i < selectedCallNodePath.length; i++) { - expandedCallNodePaths.add(selectedCallNodePath.slice(0, i)); + for (let i = 1; i < newSelectedCallNodePath.length; i++) { + expandedCallNodePaths.add(newSelectedCallNodePath.slice(0, i)); } return { ...viewOptions, - selectedCallNodePath, + selectedCallNodePath: newSelectedCallNodePath, expandedCallNodePaths, }; } diff --git a/src/test/store/actions.test.js b/src/test/store/actions.test.js index 5cc65242ce..bf11d4da8e 100644 --- a/src/test/store/actions.test.js +++ b/src/test/store/actions.test.js @@ -361,11 +361,11 @@ describe('actions/changeInvertCallstack', function () { profile, funcNamesPerThread: [funcNames], } = getProfileFromTextSamples(` - A A A A A - B E B B B - C F I I I - D G J J J - H + A A A A A A + B E B B B B + C F I I I I + D G J J J J + H K `); const toFuncIndex = (funcName) => funcNames.indexOf(funcName); const threadIndex = 0; @@ -414,7 +414,7 @@ describe('actions/changeInvertCallstack', function () { // Do not select the first alphabetical path: expect(selectedCallNodePath).not.toEqual(['D', 'C', 'B']); - // Pick the heaviest path: + // Pick the heaviest path, and stops short of K: expect(selectedCallNodePath).toEqual(['J', 'I', 'B']); expect(expandedCallNodePaths).toEqual([['J'], ['J', 'I']]); }); diff --git a/src/types/actions.js b/src/types/actions.js index c066c51376..7c5457a6c3 100644 --- a/src/types/actions.js +++ b/src/types/actions.js @@ -3,7 +3,6 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ // @flow -import { CallTree } from '../profile-logic/call-tree'; import { ReactLocalization } from '@fluent/react'; import type JSZip from 'jszip'; import type { @@ -500,8 +499,7 @@ type UrlStateAction = | {| +type: 'CHANGE_INVERT_CALLSTACK', +invertCallstack: boolean, - +callTree: CallTree, - +callNodeTable: CallNodeTable, + +newSelectedCallNodePath: CallNodePath, +selectedThreadIndexes: Set, |} | {| From bd97b7527957e97beaae59c7a420b49ea22a343d Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 20 Nov 2023 19:15:50 -0500 Subject: [PATCH 2/7] Store both the inverted and the non-inverted selected call path / expanded call paths in the redux state. --- src/actions/profile-view.js | 55 +++-- src/reducers/profile-view.js | 232 ++++++++++++------ src/selectors/per-thread/stack-sample.js | 12 +- .../__snapshots__/profile-view.test.js.snap | 12 +- src/types/actions.js | 2 + src/types/state.js | 6 +- 6 files changed, 214 insertions(+), 105 deletions(-) diff --git a/src/actions/profile-view.js b/src/actions/profile-view.js index 19c0f10155..874fcb5dd6 100644 --- a/src/actions/profile-view.js +++ b/src/actions/profile-view.js @@ -105,26 +105,30 @@ export function changeSelectedCallNode( selectedCallNodePath: CallNodePath, context: SelectionContext = { source: 'auto' }, optionalExpandedToCallNodePath?: CallNodePath -): Action { - if (optionalExpandedToCallNodePath) { - for (let i = 0; i < selectedCallNodePath.length; i++) { - if (selectedCallNodePath[i] !== optionalExpandedToCallNodePath[i]) { - // This assertion ensures that the selectedCallNode will be correctly expanded. - throw new Error( - oneLine` - The optional expanded call node path provided to the changeSelectedCallNode - must contain the selected call node path. - ` - ); +): ThunkAction { + return (dispatch, getState) => { + if (optionalExpandedToCallNodePath) { + for (let i = 0; i < selectedCallNodePath.length; i++) { + if (selectedCallNodePath[i] !== optionalExpandedToCallNodePath[i]) { + // This assertion ensures that the selectedCallNode will be correctly expanded. + throw new Error( + oneLine` + The optional expanded call node path provided to the changeSelectedCallNode + must contain the selected call node path. + ` + ); + } } } - } - return { - type: 'CHANGE_SELECTED_CALL_NODE', - selectedCallNodePath, - optionalExpandedToCallNodePath, - threadsKey, - context, + const isInverted = getInvertCallstack(getState()); + dispatch({ + type: 'CHANGE_SELECTED_CALL_NODE', + isInverted, + selectedCallNodePath, + optionalExpandedToCallNodePath, + threadsKey, + context, + }); }; } @@ -1637,14 +1641,17 @@ export function expandAllCallNodeDescendants( export function changeExpandedCallNodes( threadsKey: ThreadsKey, expandedCallNodePaths: Array -): Action { - return { - type: 'CHANGE_EXPANDED_CALL_NODES', - threadsKey, - expandedCallNodePaths, +): ThunkAction { + return (dispatch, getState) => { + const isInverted = getInvertCallstack(getState()); + dispatch({ + type: 'CHANGE_EXPANDED_CALL_NODES', + isInverted, + threadsKey, + expandedCallNodePaths, + }); }; } - export function changeSelectedMarker( threadsKey: ThreadsKey, selectedMarker: MarkerIndex | null, diff --git a/src/reducers/profile-view.js b/src/reducers/profile-view.js index fbe0262efd..139774e456 100644 --- a/src/reducers/profile-view.js +++ b/src/reducers/profile-view.js @@ -140,9 +140,11 @@ const symbolicationStatus: Reducer = ( } }; -export const defaultThreadViewOptions = { - selectedCallNodePath: [], - expandedCallNodePaths: new PathSet(), +export const defaultThreadViewOptions: ThreadViewOptions = { + selectedNonInvertedCallNodePath: [], + selectedInvertedCallNodePath: [], + expandedNonInvertedCallNodePaths: new PathSet(), + expandedInvertedCallNodePaths: new PathSet(), selectedMarker: null, selectedNetworkMarker: null, }; @@ -150,7 +152,7 @@ export const defaultThreadViewOptions = { function _getThreadViewOptions( state: ThreadViewOptionsPerThreads, threadsKey: ThreadsKey -) { +): ThreadViewOptions { const options = state[threadsKey]; if (options) { return options; @@ -202,14 +204,23 @@ const viewOptionsPerThread: Reducer = ( return { ...threadViewOptions, - selectedCallNodePath: applyFuncSubstitutionToCallPath( + selectedNonInvertedCallNodePath: applyFuncSubstitutionToCallPath( oldFuncToNewFuncsMap, - threadViewOptions.selectedCallNodePath + threadViewOptions.selectedNonInvertedCallNodePath ), - expandedCallNodePaths: + selectedInvertedCallNodePath: applyFuncSubstitutionToCallPath( + oldFuncToNewFuncsMap, + threadViewOptions.selectedInvertedCallNodePath + ), + expandedNonInvertedCallNodePaths: applyFuncSubstitutionToPathSetAndIncludeNewAncestors( oldFuncToNewFuncsMap, - threadViewOptions.expandedCallNodePaths + threadViewOptions.expandedNonInvertedCallNodePaths + ), + expandedInvertedCallNodePaths: + applyFuncSubstitutionToPathSetAndIncludeNewAncestors( + oldFuncToNewFuncsMap, + threadViewOptions.expandedInvertedCallNodePaths ), }; }); @@ -218,6 +229,7 @@ const viewOptionsPerThread: Reducer = ( } case 'CHANGE_SELECTED_CALL_NODE': { const { + isInverted, selectedCallNodePath, threadsKey, optionalExpandedToCallNodePath, @@ -225,7 +237,9 @@ const viewOptionsPerThread: Reducer = ( const threadState = _getThreadViewOptions(state, threadsKey); - const previousSelectedCallNodePath = threadState.selectedCallNodePath; + const previousSelectedCallNodePath = isInverted + ? threadState.selectedInvertedCallNodePath + : threadState.selectedNonInvertedCallNodePath; // If the selected node doesn't actually change, let's return the previous // state to avoid rerenders. @@ -236,7 +250,9 @@ const viewOptionsPerThread: Reducer = ( return state; } - let { expandedCallNodePaths } = threadState; + let expandedCallNodePaths = isInverted + ? threadState.expandedInvertedCallNodePaths + : threadState.expandedNonInvertedCallNodePaths; const expandToNode = optionalExpandedToCallNodePath ? optionalExpandedToCallNodePath : selectedCallNodePath; @@ -260,13 +276,26 @@ const viewOptionsPerThread: Reducer = ( ); } - return _updateThreadViewOptions(state, threadsKey, { - selectedCallNodePath, - expandedCallNodePaths, - }); + return _updateThreadViewOptions( + state, + threadsKey, + isInverted + ? { + selectedInvertedCallNodePath: selectedCallNodePath, + expandedInvertedCallNodePaths: expandedCallNodePaths, + } + : { + selectedNonInvertedCallNodePath: selectedCallNodePath, + expandedNonInvertedCallNodePaths: expandedCallNodePaths, + } + ); } case 'CHANGE_INVERT_CALLSTACK': { - const { newSelectedCallNodePath, selectedThreadIndexes } = action; + const { + newSelectedCallNodePath, + selectedThreadIndexes, + invertCallstack, + } = action; return objectMap(state, (viewOptions, threadsKey) => { if ( // `Object.entries` converts number threadsKeys into strings, so @@ -279,21 +308,32 @@ const viewOptionsPerThread: Reducer = ( expandedCallNodePaths.add(newSelectedCallNodePath.slice(0, i)); } - return { - ...viewOptions, - selectedCallNodePath: newSelectedCallNodePath, - expandedCallNodePaths, - }; + return invertCallstack + ? { + ...viewOptions, + selectedInvertedCallNodePath: newSelectedCallNodePath, + expandedInvertedCallNodePaths: expandedCallNodePaths, + } + : { + ...viewOptions, + selectedNonInvertedCallNodePath: newSelectedCallNodePath, + expandedNonInvertedCallNodePaths: expandedCallNodePaths, + }; } return viewOptions; }); } case 'CHANGE_EXPANDED_CALL_NODES': { - const { threadsKey, expandedCallNodePaths } = action; + const { threadsKey, isInverted } = action; + const expandedCallNodePaths = new PathSet(action.expandedCallNodePaths); - return _updateThreadViewOptions(state, threadsKey, { - expandedCallNodePaths: new PathSet(expandedCallNodePaths), - }); + return _updateThreadViewOptions( + state, + threadsKey, + isInverted + ? { expandedInvertedCallNodePaths: expandedCallNodePaths } + : { expandedNonInvertedCallNodePaths: expandedCallNodePaths } + ); } case 'CHANGE_SELECTED_MARKER': { const { threadsKey, selectedMarker } = action; @@ -308,30 +348,50 @@ const viewOptionsPerThread: Reducer = ( case 'ADD_TRANSFORM_TO_STACK': { const { threadsKey, transform, transformedThread, callNodeTable } = action; - const threadViewOptions = _getThreadViewOptions(state, threadsKey); - const expandedCallNodePaths = new PathSet( - Array.from(threadViewOptions.expandedCallNodePaths) - .map((path) => - Transforms.applyTransformToCallNodePath( - path, - transform, - transformedThread, - callNodeTable + + const getFilteredPathSet = function (pathSet: PathSet): PathSet { + return new PathSet( + Array.from(pathSet) + .map((path) => + Transforms.applyTransformToCallNodePath( + path, + transform, + transformedThread, + callNodeTable + ) ) - ) - .filter((path) => path.length > 0) - ); + .filter((path) => path.length > 0) + ); + }; - const selectedCallNodePath = Transforms.applyTransformToCallNodePath( - threadViewOptions.selectedCallNodePath, - transform, - transformedThread, - callNodeTable + const getFilteredPath = function (path: CallNodePath): CallNodePath { + return Transforms.applyTransformToCallNodePath( + path, + transform, + transformedThread, + callNodeTable + ); + }; + + const threadViewOptions = _getThreadViewOptions(state, threadsKey); + const selectedNonInvertedCallNodePath = getFilteredPath( + threadViewOptions.selectedNonInvertedCallNodePath + ); + const selectedInvertedCallNodePath = getFilteredPath( + threadViewOptions.selectedInvertedCallNodePath + ); + const expandedNonInvertedCallNodePaths = getFilteredPathSet( + threadViewOptions.expandedNonInvertedCallNodePaths + ); + const expandedInvertedCallNodePaths = getFilteredPathSet( + threadViewOptions.expandedInvertedCallNodePaths ); return _updateThreadViewOptions(state, threadsKey, { - selectedCallNodePath, - expandedCallNodePaths, + selectedNonInvertedCallNodePath, + selectedInvertedCallNodePath, + expandedNonInvertedCallNodePaths, + expandedInvertedCallNodePaths, }); } case 'POP_TRANSFORMS_FROM_STACK': { @@ -339,8 +399,10 @@ const viewOptionsPerThread: Reducer = ( // https://github.com/firefox-devtools/profiler/issues/882 const { threadsKey } = action; return _updateThreadViewOptions(state, threadsKey, { - selectedCallNodePath: [], - expandedCallNodePaths: new PathSet(), + selectedNonInvertedCallNodePath: [], + selectedInvertedCallNodePath: [], + expandedNonInvertedCallNodePaths: new PathSet(), + expandedInvertedCallNodePaths: new PathSet(), }); } case 'CHANGE_IMPLEMENTATION_FILTER': { @@ -357,41 +419,65 @@ const viewOptionsPerThread: Reducer = ( const viewOptions = _getThreadViewOptions(state, threadsKey); - // This CallNodePath may need to be updated twice. - let selectedCallNodePath: CallNodePath = viewOptions.selectedCallNodePath; - if (implementation === 'combined') { - // Restore the full CallNodePaths - selectedCallNodePath = Transforms.restoreAllFunctionsInCallNodePath( - transformedThread, - previousImplementation, - selectedCallNodePath - ); - } else { - if (previousImplementation !== 'combined') { - // Restore the CallNodePath back to an unfiltered state before re-filtering - // it on the next implementation. - selectedCallNodePath = Transforms.restoreAllFunctionsInCallNodePath( + const getUpdatedPath = function getUpdatedPath( + callNodePath: CallNodePath + ): CallNodePath { + // This CallNodePath may need to be updated twice. + if (implementation === 'combined') { + // Restore the full CallNodePaths + callNodePath = Transforms.restoreAllFunctionsInCallNodePath( transformedThread, previousImplementation, - selectedCallNodePath + callNodePath + ); + } else { + if (previousImplementation !== 'combined') { + // Restore the CallNodePath back to an unfiltered state before re-filtering + // it on the next implementation. + callNodePath = Transforms.restoreAllFunctionsInCallNodePath( + transformedThread, + previousImplementation, + callNodePath + ); + } + // Take the full CallNodePath, and strip out anything not in this implementation. + callNodePath = Transforms.filterCallNodePathByImplementation( + transformedThread, + implementation, + callNodePath ); } - // Take the full CallNodePath, and strip out anything not in this implementation. - selectedCallNodePath = Transforms.filterCallNodePathByImplementation( - transformedThread, - implementation, - selectedCallNodePath - ); - } + return callNodePath; + }; - const expandedCallNodePaths = new PathSet(); - for (let i = 1; i < selectedCallNodePath.length; i++) { - expandedCallNodePaths.add(selectedCallNodePath.slice(0, i)); - } + const getAncestorPathSet = function getAncestorPathSet( + callNodePath: CallNodePath + ): PathSet { + const ancestorCallNodePaths = new PathSet(); + for (let i = 1; i < callNodePath.length; i++) { + ancestorCallNodePaths.add(callNodePath.slice(0, i)); + } + return ancestorCallNodePaths; + }; + + const selectedNonInvertedCallNodePath = getUpdatedPath( + viewOptions.selectedNonInvertedCallNodePath + ); + const selectedInvertedCallNodePath = getUpdatedPath( + viewOptions.selectedInvertedCallNodePath + ); + const expandedNonInvertedCallNodePaths = getAncestorPathSet( + selectedNonInvertedCallNodePath + ); + const expandedInvertedCallNodePaths = getAncestorPathSet( + selectedInvertedCallNodePath + ); return _updateThreadViewOptions(state, threadsKey, { - selectedCallNodePath, - expandedCallNodePaths, + selectedNonInvertedCallNodePath, + selectedInvertedCallNodePath, + expandedNonInvertedCallNodePaths, + expandedInvertedCallNodePaths, }); } default: diff --git a/src/selectors/per-thread/stack-sample.js b/src/selectors/per-thread/stack-sample.js index 92b265443f..3ffdb954de 100644 --- a/src/selectors/per-thread/stack-sample.js +++ b/src/selectors/per-thread/stack-sample.js @@ -178,7 +178,11 @@ export function getStackAndSampleSelectorsPerThread( const getSelectedCallNodePath: Selector = createSelector( threadSelectors.getViewOptions, - (threadViewOptions): CallNodePath => threadViewOptions.selectedCallNodePath + UrlState.getInvertCallstack, + (threadViewOptions, invertCallStack): CallNodePath => + invertCallStack + ? threadViewOptions.selectedInvertedCallNodePath + : threadViewOptions.selectedNonInvertedCallNodePath ); const getSelectedCallNodeIndex: Selector = @@ -195,7 +199,11 @@ export function getStackAndSampleSelectorsPerThread( const getExpandedCallNodePaths: Selector = createSelector( threadSelectors.getViewOptions, - (threadViewOptions) => threadViewOptions.expandedCallNodePaths + UrlState.getInvertCallstack, + (threadViewOptions, invertCallStack) => + invertCallStack + ? threadViewOptions.expandedInvertedCallNodePaths + : threadViewOptions.expandedNonInvertedCallNodePaths ); const getExpandedCallNodeIndexes: Selector< diff --git a/src/test/store/__snapshots__/profile-view.test.js.snap b/src/test/store/__snapshots__/profile-view.test.js.snap index 14f77d33cb..eb39ebb769 100644 --- a/src/test/store/__snapshots__/profile-view.test.js.snap +++ b/src/test/store/__snapshots__/profile-view.test.js.snap @@ -4694,7 +4694,10 @@ Process: \\"default\\" (0)" exports[`snapshots of selectors/profile matches the last stored run of selectedThreadSelector.getViewOptions 1`] = ` Object { - "expandedCallNodePaths": PathSet { + "expandedInvertedCallNodePaths": PathSet { + "_table": Map {}, + }, + "expandedNonInvertedCallNodePaths": PathSet { "_table": Map { "0" => Array [ 0, @@ -4705,11 +4708,12 @@ Object { ], }, }, - "selectedCallNodePath": Array [ + "selectedInvertedCallNodePath": Array [], + "selectedMarker": 1, + "selectedNetworkMarker": null, + "selectedNonInvertedCallNodePath": Array [ 0, 1, ], - "selectedMarker": 1, - "selectedNetworkMarker": null, } `; diff --git a/src/types/actions.js b/src/types/actions.js index 7c5457a6c3..a771fcfc88 100644 --- a/src/types/actions.js +++ b/src/types/actions.js @@ -195,6 +195,7 @@ type ProfileAction = |} | {| +type: 'CHANGE_SELECTED_CALL_NODE', + +isInverted: boolean, +threadsKey: ThreadsKey, +selectedCallNodePath: CallNodePath, +optionalExpandedToCallNodePath: ?CallNodePath, @@ -216,6 +217,7 @@ type ProfileAction = | {| +type: 'CHANGE_EXPANDED_CALL_NODES', +threadsKey: ThreadsKey, + +isInverted: boolean, +expandedCallNodePaths: Array, |} | {| diff --git a/src/types/state.js b/src/types/state.js index beccd8345c..f85dcb5d6d 100644 --- a/src/types/state.js +++ b/src/types/state.js @@ -56,8 +56,10 @@ export type UploadedProfileInformation = ImportedUploadedProfileInformation; export type SymbolicationStatus = 'DONE' | 'SYMBOLICATING'; export type ThreadViewOptions = {| - +selectedCallNodePath: CallNodePath, - +expandedCallNodePaths: PathSet, + +selectedNonInvertedCallNodePath: CallNodePath, + +selectedInvertedCallNodePath: CallNodePath, + +expandedNonInvertedCallNodePaths: PathSet, + +expandedInvertedCallNodePaths: PathSet, +selectedMarker: MarkerIndex | null, +selectedNetworkMarker: MarkerIndex | null, |}; From 6dd845eddd4e9452f41e2c4657fdcfcc67bfacea Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 20 Nov 2023 19:44:40 -0500 Subject: [PATCH 3/7] Only respect the invertCallstack state in the call tree tab. This means that the flame graph and the stack chart will now always display un-inverted. Fixes #3961. Fixes #4803. --- src/selectors/url-state.js | 7 ++++--- src/test/components/FlameGraph.test.js | 17 +++++++---------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/selectors/url-state.js b/src/selectors/url-state.js index 59ff65febc..e9462386d4 100644 --- a/src/selectors/url-state.js +++ b/src/selectors/url-state.js @@ -73,8 +73,6 @@ export const getLastSelectedCallTreeSummaryStrategy: Selector< CallTreeSummaryStrategy, > = (state) => getProfileSpecificState(state).lastSelectedCallTreeSummaryStrategy; -export const getInvertCallstack: Selector = (state) => - getProfileSpecificState(state).invertCallstack; export const getShowUserTimings: Selector = (state) => getProfileSpecificState(state).showUserTimings; export const getSourceViewFile: Selector = (state) => @@ -109,9 +107,12 @@ export const getMarkersSearchString: Selector = (state) => getProfileSpecificState(state).markersSearchString; export const getNetworkSearchString: Selector = (state) => getProfileSpecificState(state).networkSearchString; - export const getSelectedTab: Selector = (state) => getUrlState(state).selectedTab; +export const getInvertCallstack: Selector = (state) => + getSelectedTab(state) === 'calltree' && + getProfileSpecificState(state).invertCallstack; + export const getSelectedThreadIndexesOrNull: Selector< Set | null, > = (state) => getProfileSpecificState(state).selectedThreads; diff --git a/src/test/components/FlameGraph.test.js b/src/test/components/FlameGraph.test.js index 91e5cf00e5..33e7aac787 100644 --- a/src/test/components/FlameGraph.test.js +++ b/src/test/components/FlameGraph.test.js @@ -29,6 +29,7 @@ import { updatePreviewSelection, changeImplementationFilter, } from '../../actions/profile-view'; +import { changeSelectedTab } from '../../actions/app'; import { selectedThreadSelectors } from '../../selectors/per-thread'; import { @@ -67,17 +68,12 @@ describe('FlameGraph', function () { expect(drawCalls).toMatchSnapshot(); }); - it('renders a message instead of the graph when call stack is inverted', () => { - const { getByText, dispatch } = setupFlameGraph(); - dispatch(changeInvertCallstack(true)); - expect(getByText(/The Flame Graph is not available/)).toBeInTheDocument(); - }); - - it('switches back to uninverted mode when clicking the button', () => { - const { getByText, dispatch, getState } = setupFlameGraph(); + it('ignores invertCallstack and always displays non-inverted', () => { + const { getState, dispatch } = setupFlameGraph(); + expect(getInvertCallstack(getState())).toBe(false); dispatch(changeInvertCallstack(true)); - expect(getInvertCallstack(getState())).toBe(true); - fireFullClick(getByText(/Switch to the normal call stack/)); + expect(getInvertCallstack(getState())).toBe(false); + dispatch(changeInvertCallstack(false)); expect(getInvertCallstack(getState())).toBe(false); }); @@ -298,6 +294,7 @@ function setupFlameGraph(addImplementationData: boolean = true) { } const store = storeWithProfile(profile); + store.dispatch(changeSelectedTab('flame-graph')); const renderResult = render( From ac4a1ac4ff84122b55c23de7b43649eed48da726 Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 20 Nov 2023 20:26:14 -0500 Subject: [PATCH 4/7] Add a test which checks that selection is tracked independently for inverted and non-inverted views. --- src/test/store/profile-view.test.js | 63 +++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/src/test/store/profile-view.test.js b/src/test/store/profile-view.test.js index 7be56c4a4a..d01d34be06 100644 --- a/src/test/store/profile-view.test.js +++ b/src/test/store/profile-view.test.js @@ -1933,6 +1933,69 @@ describe('snapshots of selectors/profile', function () { }); }); +describe('changeSelectedCallNode', function () { + it('switching between the call tree and the flame graph always selects a reasonable node', function () { + const { + profile, + funcNamesDictPerThread: [funcNamesDict], + } = getProfileFromTextSamples(` + A A A A + B B B B + C C C H + D D F I + E E G + `); + + const { A, B, C, D, E, F } = funcNamesDict; + + const { dispatch, getState } = storeWithProfile(profile); + + dispatch(App.changeSelectedTab('calltree')); + dispatch(ProfileView.changeSelectedCallNode(0, [A, B, C])); + expect(selectedThreadSelectors.getSelectedCallNodePath(getState())).toEqual( + [A, B, C] + ); + dispatch(ProfileView.changeInvertCallstack(true)); + expect(UrlStateSelectors.getInvertCallstack(getState())).toEqual(true); + + // Inverting the call stack should have picked the heaviest inverted stack + // as the new selected call node path. + expect(selectedThreadSelectors.getSelectedCallNodePath(getState())).toEqual( + [E, D, C] + ); + + dispatch(App.changeSelectedTab('flame-graph')); + // In the flame graph, everything should still be non-inverted. + expect(UrlStateSelectors.getInvertCallstack(getState())).toEqual(false); + // The original non-inverted selected call node should be selected. + expect(selectedThreadSelectors.getSelectedCallNodePath(getState())).toEqual( + [A, B, C] + ); + + // Now we select a different call node in the flame graph. + dispatch(ProfileView.changeSelectedCallNode(0, [A, B, C, F])); + expect(selectedThreadSelectors.getSelectedCallNodePath(getState())).toEqual( + [A, B, C, F] + ); + + // Switch back to the call tree tab. In the call tree tab, we should still + // be looking at the inverted tree, with the unchanged inverted selection. + dispatch(App.changeSelectedTab('calltree')); + expect(UrlStateSelectors.getInvertCallstack(getState())).toEqual(true); + expect(selectedThreadSelectors.getSelectedCallNodePath(getState())).toEqual( + [E, D, C] + ); + + // Switching back to non-inverted mode should pick a new non-inverted + // selected call node based on the selection in the inverted tree. + dispatch(ProfileView.changeInvertCallstack(false)); + expect(UrlStateSelectors.getInvertCallstack(getState())).toEqual(false); + expect(selectedThreadSelectors.getSelectedCallNodePath(getState())).toEqual( + [A, B, C] + ); + }); +}); + describe('getTimingsForSidebar', () => { function getGenericProfileString() { // Note that the first column won't be counted because a range is used, From f259c8cb53035e3c972d6c2151fd169cb7ba473a Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Mon, 20 Nov 2023 20:31:09 -0500 Subject: [PATCH 5/7] Remove non-functional "Invert call stack" checkbox from the stack chart tab. --- src/components/stack-chart/index.js | 2 +- .../__snapshots__/StackChart.test.js.snap | 39 ------------------- 2 files changed, 1 insertion(+), 40 deletions(-) diff --git a/src/components/stack-chart/index.js b/src/components/stack-chart/index.js index 9f88a953f8..1e5990bc83 100644 --- a/src/components/stack-chart/index.js +++ b/src/components/stack-chart/index.js @@ -238,7 +238,7 @@ class StackChartImpl extends React.PureComponent { role="tabpanel" aria-labelledby="stack-chart-tab-button" > - + {maxStackDepth === 0 && userTimings.length === 0 ? ( diff --git a/src/test/components/__snapshots__/StackChart.test.js.snap b/src/test/components/__snapshots__/StackChart.test.js.snap index acfe572952..d73e7e7b9d 100644 --- a/src/test/components/__snapshots__/StackChart.test.js.snap +++ b/src/test/components/__snapshots__/StackChart.test.js.snap @@ -63,19 +63,6 @@ exports[`CombinedChart renders combined stack chart 1`] = `
  • -
  • -
  • -