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

Make inverting the call tree fast, by computing inverted call nodes lazily #4900

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/actions/profile-view.js
Original file line number Diff line number Diff line change
Expand Up @@ -2035,12 +2035,13 @@ export function handleCallNodeTransformShortcut(
const threadSelectors = getThreadSelectorsFromThreadsKey(threadsKey);
const unfilteredThread = threadSelectors.getThread(getState());
const callNodeInfo = threadSelectors.getCallNodeInfo(getState());
const callNodeTable = callNodeInfo.getCallNodeTable();
const implementation = getImplementationFilter(getState());
const inverted = getInvertCallstack(getState());
const callNodePath = callNodeInfo.getCallNodePathFromIndex(callNodeIndex);
const funcIndex = callNodeTable.func[callNodeIndex];
const category = callNodeTable.category[callNodeIndex];
const funcIndex = callNodeInfo.funcForNode(callNodeIndex);
const category = callNodeInfo.categoryForNode(callNodeIndex);

const nonInvertedCallNodeTable = callNodeInfo.getNonInvertedCallNodeTable();

switch (event.key) {
case 'F':
Expand Down Expand Up @@ -2099,7 +2100,7 @@ export function handleCallNodeTransformShortcut(
break;
}
case 'r': {
if (funcHasRecursiveCall(callNodeTable, funcIndex)) {
if (funcHasRecursiveCall(nonInvertedCallNodeTable, funcIndex)) {
dispatch(
addTransformToStack(threadsKey, {
type: 'collapse-recursion',
Expand All @@ -2110,7 +2111,7 @@ export function handleCallNodeTransformShortcut(
break;
}
case 'R': {
if (funcHasDirectRecursiveCall(callNodeTable, funcIndex)) {
if (funcHasDirectRecursiveCall(nonInvertedCallNodeTable, funcIndex)) {
dispatch(
addTransformToStack(threadsKey, {
type: 'collapse-direct-recursion',
Expand Down
6 changes: 3 additions & 3 deletions src/components/calltree/CallTree.js
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,6 @@ class CallTreeImpl extends PureComponent<Props> {
// This tree is empty.
return;
}
const callNodeTable = callNodeInfo.getCallNodeTable();
newExpandedCallNodeIndexes.push(currentCallNodeIndex);
for (let i = 0; i < maxInterestingDepth; i++) {
const children = tree.getChildren(currentCallNodeIndex);
Expand All @@ -330,7 +329,8 @@ class CallTreeImpl extends PureComponent<Props> {

// Let's find if there's a non idle children.
const firstNonIdleNode = children.find(
(nodeIndex) => callNodeTable.category[nodeIndex] !== idleCategoryIndex
(nodeIndex) =>
callNodeInfo.categoryForNode(nodeIndex) !== idleCategoryIndex
);

// If there's a non idle children, use it; otherwise use the first
Expand All @@ -341,7 +341,7 @@ class CallTreeImpl extends PureComponent<Props> {
}
this._onExpandedCallNodesChange(newExpandedCallNodeIndexes);

const categoryIndex = callNodeTable.category[currentCallNodeIndex];
const categoryIndex = callNodeInfo.categoryForNode(currentCallNodeIndex);
if (categoryIndex !== idleCategoryIndex) {
// 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
Expand Down
4 changes: 2 additions & 2 deletions src/components/flame-graph/Canvas.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ import type {

import type {
CallTree,
CallTreeTimings,
CallTreeTimingsNonInverted,
} from 'firefox-profiler/profile-logic/call-tree';

export type OwnProps = {|
Expand All @@ -77,7 +77,7 @@ export type OwnProps = {|
+callTreeSummaryStrategy: CallTreeSummaryStrategy,
+samples: SamplesLikeTable,
+unfilteredSamples: SamplesLikeTable,
+tracedTiming: CallTreeTimings | null,
+tracedTiming: CallTreeTimingsNonInverted | null,
+displayImplementation: boolean,
+displayStackType: boolean,
|};
Expand Down
7 changes: 6 additions & 1 deletion src/components/flame-graph/FlameGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,11 @@
displayStackType,
} = this.props;

const tracedTimingNonInverted =
tracedTiming !== null && tracedTiming.type === 'NON_INVERTED'
? tracedTiming.timings
: null;

Check warning on line 350 in src/components/flame-graph/FlameGraph.js

View check run for this annotation

Codecov / codecov/patch

src/components/flame-graph/FlameGraph.js#L350

Added line #L350 was not covered by tests
Comment on lines +347 to +350
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it happen for the flame graph that we have timings of inverted type?
And if this happens (in an hypothetical future), it means we'd want to show the inverted data as a flame graph, in that case I think this condition isn't useful, we could simply pass tracedTiming.timings always. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I understand you did it that way because the timings object is widely different for inverted vs non-inverted trees.


const maxViewportHeight = maxStackDepthPlusOne * STACK_FRAME_HEIGHT;

return (
Expand Down Expand Up @@ -394,7 +399,7 @@
isInverted,
samples,
unfilteredSamples,
tracedTiming,
tracedTiming: tracedTimingNonInverted,
displayImplementation,
displayStackType,
}}
Expand Down
17 changes: 8 additions & 9 deletions src/components/shared/CallNodeContextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,7 @@ class CallNodeContextMenuImpl extends React.PureComponent<Props> {
callNodeInfo,
} = rightClickedCallNodeInfo;

const callNodeTable = callNodeInfo.getCallNodeTable();
const funcIndex = callNodeTable.func[callNodeIndex];
const funcIndex = callNodeInfo.funcForNode(callNodeIndex);
const isJS = funcTable.isJS[funcIndex];
const stringIndex = funcTable.name[funcIndex];
const functionCall = stringTable.getString(stringIndex);
Expand Down Expand Up @@ -176,8 +175,7 @@ class CallNodeContextMenuImpl extends React.PureComponent<Props> {
callNodeInfo,
} = rightClickedCallNodeInfo;

const callNodeTable = callNodeInfo.getCallNodeTable();
const funcIndex = callNodeTable.func[callNodeIndex];
const funcIndex = callNodeInfo.funcForNode(callNodeIndex);
const stringIndex = funcTable.fileName[funcIndex];
if (stringIndex === null) {
return null;
Expand Down Expand Up @@ -303,8 +301,7 @@ class CallNodeContextMenuImpl extends React.PureComponent<Props> {
const { threadsKey, callNodePath, thread, callNodeIndex, callNodeInfo } =
rightClickedCallNodeInfo;
const selectedFunc = callNodePath[callNodePath.length - 1];
const callNodeTable = callNodeInfo.getCallNodeTable();
const category = callNodeTable.category[callNodeIndex];
const category = callNodeInfo.categoryForNode(callNodeIndex);
switch (type) {
case 'focus-subtree':
addTransformToStack(threadsKey, {
Expand Down Expand Up @@ -488,9 +485,8 @@ class CallNodeContextMenuImpl extends React.PureComponent<Props> {
callNodeInfo,
} = rightClickedCallNodeInfo;

const callNodeTable = callNodeInfo.getCallNodeTable();
const categoryIndex = callNodeTable.category[callNodeIndex];
const funcIndex = callNodeTable.func[callNodeIndex];
const categoryIndex = callNodeInfo.categoryForNode(callNodeIndex);
const funcIndex = callNodeInfo.funcForNode(callNodeIndex);
const isJS = funcTable.isJS[funcIndex];
const hasCategory = categoryIndex !== -1;
// This could be the C++ library, or the JS filename.
Expand All @@ -504,6 +500,9 @@ class CallNodeContextMenuImpl extends React.PureComponent<Props> {
const fileName =
filePath &&
parseFileNameFromSymbolication(filePath).path.match(/[^\\/]+$/)?.[0];

const callNodeTable = callNodeInfo.getNonInvertedCallNodeTable();

return (
<>
{fileName ? (
Expand Down
5 changes: 2 additions & 3 deletions src/components/stack-chart/Canvas.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,7 @@ class StackChartCanvasImpl extends React.PureComponent<Props> {
return;
}

const callNodeTable = callNodeInfo.getCallNodeTable();
const depth = callNodeTable.depth[selectedCallNodeIndex];
const depth = callNodeInfo.depthForNode(selectedCallNodeIndex);
const y = depth * ROW_CSS_PIXELS_HEIGHT;

if (y < this.props.viewport.viewportTop) {
Expand Down Expand Up @@ -262,7 +261,7 @@ class StackChartCanvasImpl extends React.PureComponent<Props> {
categoryForUserTiming = 0;
}

const callNodeTable = callNodeInfo.getCallNodeTable();
const callNodeTable = callNodeInfo.getNonInvertedCallNodeTable();

// Only draw the stack frames that are vertically within view.
for (let depth = startDepth; depth < endDepth; depth++) {
Expand Down
3 changes: 1 addition & 2 deletions src/components/stack-chart/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,7 @@
event.preventDefault();
const { callNodeInfo, selectedCallNodeIndex, thread } = this.props;
if (selectedCallNodeIndex !== null) {
const callNodeTable = callNodeInfo.getCallNodeTable();
const funcIndex = callNodeTable.func[selectedCallNodeIndex];
const funcIndex = callNodeInfo.funcForNode(selectedCallNodeIndex);

Check warning on line 184 in src/components/stack-chart/index.js

View check run for this annotation

Codecov / codecov/patch

src/components/stack-chart/index.js#L184

Added line #L184 was not covered by tests
const funcName = thread.stringTable.getString(
thread.funcTable.name[funcIndex]
);
Expand Down
9 changes: 4 additions & 5 deletions src/components/tooltip/CallNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -365,12 +365,11 @@ export class TooltipCallNode extends React.PureComponent<Props> {
callNodeInfo,
displayStackType,
} = this.props;
const callNodeTable = callNodeInfo.getCallNodeTable();
const categoryIndex = callNodeTable.category[callNodeIndex];
const categoryIndex = callNodeInfo.categoryForNode(callNodeIndex);
const categoryColor = categories[categoryIndex].color;
const subcategoryIndex = callNodeTable.subcategory[callNodeIndex];
const funcIndex = callNodeTable.func[callNodeIndex];
const innerWindowID = callNodeTable.innerWindowID[callNodeIndex];
const subcategoryIndex = callNodeInfo.subcategoryForNode(callNodeIndex);
const funcIndex = callNodeInfo.funcForNode(callNodeIndex);
const innerWindowID = callNodeInfo.innerWindowIDForNode(callNodeIndex);
const funcStringIndex = thread.funcTable.name[funcIndex];
const funcName = thread.stringTable.getString(funcStringIndex);

Expand Down
26 changes: 15 additions & 11 deletions src/profile-logic/address-timings.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ import type {
StackTable,
SamplesLikeTable,
CallNodeInfo,
CallNodeInfoInverted,
IndexIntoCallNodeTable,
IndexIntoNativeSymbolTable,
StackAddressInfo,
Expand Down Expand Up @@ -202,12 +203,13 @@ export function getStackAddressInfoForCallNode(
callNodeInfo: CallNodeInfo,
nativeSymbol: IndexIntoNativeSymbolTable
): StackAddressInfo {
return callNodeInfo.isInverted()
const callNodeInfoInverted = callNodeInfo.asInverted();
return callNodeInfoInverted !== null
? getStackAddressInfoForCallNodeInverted(
stackTable,
frameTable,
callNodeIndex,
callNodeInfo,
callNodeInfoInverted,
nativeSymbol
)
: getStackAddressInfoForCallNodeNonInverted(
Expand Down Expand Up @@ -426,16 +428,17 @@ export function getStackAddressInfoForCallNodeInverted(
stackTable: StackTable,
frameTable: FrameTable,
callNodeIndex: IndexIntoCallNodeTable,
callNodeInfo: CallNodeInfo,
callNodeInfo: CallNodeInfoInverted,
nativeSymbol: IndexIntoNativeSymbolTable
): StackAddressInfo {
const invertedCallNodeTable = callNodeInfo.getCallNodeTable();
const depth = invertedCallNodeTable.depth[callNodeIndex];
const endIndex = invertedCallNodeTable.subtreeRangeEnd[callNodeIndex];
const callNodeIsRootOfInvertedTree =
invertedCallNodeTable.prefix[callNodeIndex] === -1;
const stackIndexToCallNodeIndex = callNodeInfo.getStackIndexToCallNodeIndex();
const depth = callNodeInfo.depthForNode(callNodeIndex);
const [rangeStart, rangeEnd] =
callNodeInfo.getSuffixOrderIndexRangeForCallNode(callNodeIndex);
const callNodeIsRootOfInvertedTree = callNodeInfo.isRoot(callNodeIndex);
const stackIndexToCallNodeIndex =
callNodeInfo.getStackIndexToNonInvertedCallNodeIndex();
const stackTablePrefixCol = stackTable.prefix;
const suffixOrderIndexes = callNodeInfo.getSuffixOrderIndexes();

// "self address" == "the address which a stack's self time is contributed to"
const callNodeSelfAddressForAllStacks = [];
Expand All @@ -449,8 +452,9 @@ export function getStackAddressInfoForCallNodeInverted(

const stackForCallNode = getMatchingAncestorStackForInvertedCallNode(
stackIndex,
callNodeIndex,
endIndex,
rangeStart,
rangeEnd,
suffixOrderIndexes,
depth,
stackIndexToCallNodeIndex,
stackTablePrefixCol
Expand Down
Loading