-
Notifications
You must be signed in to change notification settings - Fork 391
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
Add a "Merge unaccounted native frames" transform #5141
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ import { parseFileNameFromSymbolication } from 'firefox-profiler/utils/special-p | |
import { | ||
funcHasDirectRecursiveCall, | ||
funcHasRecursiveCall, | ||
isUnaccountedNativeFunction, | ||
} from 'firefox-profiler/profile-logic/transforms'; | ||
import { getFunctionName } from 'firefox-profiler/profile-logic/function-info'; | ||
import { | ||
|
@@ -333,6 +334,11 @@ class CallNodeContextMenuImpl extends React.PureComponent<Props> { | |
funcIndex: selectedFunc, | ||
}); | ||
break; | ||
case 'merge-unaccounted-native-functions': | ||
addTransformToStack(threadsKey, { | ||
type: 'merge-unaccounted-native-functions', | ||
}); | ||
break; | ||
case 'drop-function': | ||
addTransformToStack(threadsKey, { | ||
type: 'drop-function', | ||
|
@@ -484,7 +490,7 @@ class CallNodeContextMenuImpl extends React.PureComponent<Props> { | |
|
||
const { | ||
callNodeIndex, | ||
thread: { funcTable }, | ||
thread: { funcTable, stringTable }, | ||
callNodeInfo, | ||
} = rightClickedCallNodeInfo; | ||
|
||
|
@@ -504,6 +510,11 @@ class CallNodeContextMenuImpl extends React.PureComponent<Props> { | |
const fileName = | ||
filePath && | ||
parseFileNameFromSymbolication(filePath).path.match(/[^\\/]+$/)?.[0]; | ||
const isProbablyJIT = isUnaccountedNativeFunction( | ||
funcIndex, | ||
funcTable, | ||
stringTable | ||
); | ||
return ( | ||
<> | ||
{fileName ? ( | ||
|
@@ -545,6 +556,19 @@ class CallNodeContextMenuImpl extends React.PureComponent<Props> { | |
content: 'Merge node only', | ||
})} | ||
|
||
{isProbablyJIT | ||
? this.renderTransformMenuItem({ | ||
l10nId: | ||
'CallNodeContextMenu--transform-merge-unaccounted-native-functions', | ||
shortcut: 'J', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you need to handle this shortcut to May I ask why |
||
icon: 'Merge', | ||
onClick: this._handleClick, | ||
transform: 'merge-unaccounted-native-functions', | ||
title: '', | ||
content: 'Merge unaccounted native frames', | ||
}) | ||
: null} | ||
|
||
{this.renderTransformMenuItem({ | ||
l10nId: inverted | ||
? 'CallNodeContextMenu--transform-focus-function-inverted' | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -65,6 +65,7 @@ const SHORT_KEY_TO_TRANSFORM: { [string]: TransformType } = {}; | |||||
'focus-category', | ||||||
'merge-call-node', | ||||||
'merge-function', | ||||||
'merge-unaccounted-native-functions', | ||||||
'drop-function', | ||||||
'collapse-resource', | ||||||
'collapse-direct-recursion', | ||||||
|
@@ -91,6 +92,9 @@ const SHORT_KEY_TO_TRANSFORM: { [string]: TransformType } = {}; | |||||
case 'merge-function': | ||||||
shortKey = 'mf'; | ||||||
break; | ||||||
case 'merge-unaccounted-native-functions': | ||||||
shortKey = 'munfs'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (optional) not a super big deal, but what about simply |
||||||
break; | ||||||
case 'drop-function': | ||||||
shortKey = 'df'; | ||||||
break; | ||||||
|
@@ -235,6 +239,12 @@ export function parseTransforms(transformString: string): TransformStack { | |||||
} | ||||||
break; | ||||||
} | ||||||
case 'merge-unaccounted-native-functions': { | ||||||
transforms.push({ | ||||||
type: 'merge-unaccounted-native-functions', | ||||||
}); | ||||||
break; | ||||||
} | ||||||
case 'focus-category': { | ||||||
// e.g. "fg-3" | ||||||
const [, categoryRaw] = tuple; | ||||||
|
@@ -348,6 +358,8 @@ export function stringifyTransforms(transformStack: TransformStack): string { | |||||
case 'collapse-function-subtree': | ||||||
case 'focus-function': | ||||||
return `${shortKey}-${transform.funcIndex}`; | ||||||
case 'merge-unaccounted-native-functions': | ||||||
return shortKey; | ||||||
case 'focus-category': | ||||||
return `${shortKey}-${transform.category}`; | ||||||
case 'collapse-resource': | ||||||
|
@@ -432,6 +444,13 @@ export function getTransformLabelL10nIds( | |||||
} | ||||||
} | ||||||
|
||||||
if (transform.type === 'merge-unaccounted-native-functions') { | ||||||
return { | ||||||
l10nId: 'TransformNavigator--merge-unaccounted-native-functions', | ||||||
item: '', | ||||||
}; | ||||||
} | ||||||
|
||||||
// Lookup function name. | ||||||
let funcIndex; | ||||||
switch (transform.type) { | ||||||
|
@@ -517,6 +536,12 @@ export function applyTransformToCallNodePath( | |||||
return _mergeNodeInCallNodePath(transform.callNodePath, callNodePath); | ||||||
case 'merge-function': | ||||||
return _mergeFunctionInCallNodePath(transform.funcIndex, callNodePath); | ||||||
case 'merge-unaccounted-native-functions': | ||||||
return _mergeUnaccountedNativeFunctionsInCallNodePath( | ||||||
callNodePath, | ||||||
transformedThread.funcTable, | ||||||
transformedThread.stringTable | ||||||
); | ||||||
case 'drop-function': | ||||||
return _dropFunctionInCallNodePath(transform.funcIndex, callNodePath); | ||||||
case 'collapse-resource': | ||||||
|
@@ -588,6 +613,38 @@ function _mergeFunctionInCallNodePath( | |||||
return callNodePath.filter((nodeFunc) => nodeFunc !== funcIndex); | ||||||
} | ||||||
|
||||||
function _mergeUnaccountedNativeFunctionsInCallNodePath( | ||||||
callNodePath: CallNodePath, | ||||||
funcTable: FuncTable, | ||||||
stringTable: UniqueStringArray | ||||||
): CallNodePath { | ||||||
return callNodePath.filter((nodeFunc) => | ||||||
isUnaccountedNativeFunction(nodeFunc, funcTable, stringTable) | ||||||
); | ||||||
} | ||||||
|
||||||
// Returns true if funcIndex is probably a JIT frame outside of any known JIT | ||||||
// address mappings. | ||||||
export function isUnaccountedNativeFunction( | ||||||
funcIndex: IndexIntoFuncTable, | ||||||
funcTable: FuncTable, | ||||||
stringTable: UniqueStringArray | ||||||
): boolean { | ||||||
if (funcTable.resource[funcIndex] !== -1) { | ||||||
// This function has a resource. That means it's not "unaccounted". | ||||||
return false; | ||||||
} | ||||||
if (funcTable.isJS[funcIndex]) { | ||||||
// This function is a JS function. That means it's not a "native" function. | ||||||
return false; | ||||||
} | ||||||
// Ok, so now we either have a native function without a library (otherwise it | ||||||
// would have a "lib" resource), or we have a label frame. Assume that label | ||||||
// frames don't start with "0x". | ||||||
const locationString = stringTable.getString(funcTable.name[funcIndex]); | ||||||
return locationString.startsWith('0x'); | ||||||
} | ||||||
|
||||||
function _dropFunctionInCallNodePath( | ||||||
funcIndex: IndexIntoFuncTable, | ||||||
callNodePath: CallNodePath | ||||||
|
@@ -859,13 +916,96 @@ export function mergeFunction( | |||||
}); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Returns a Uint8Array filled with zeros and ones. | ||||||
* result[funcIndex] === 1 iff isUnaccountedNativeFunction(funcIndex) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh yeah okay, got it now. All my maths were done in french so I didn't think of that :-) |
||||||
*/ | ||||||
function getUnaccountedNativeFunctions(thread: Thread): Uint8Array { | ||||||
const { funcTable, stringTable } = thread; | ||||||
const funcCount = funcTable.length; | ||||||
const { isJS: funcIsJS, resource: funcResource, name: funcName } = funcTable; | ||||||
const isUnaccountedNativeFunctionArr = new Uint8Array(funcTable.length); | ||||||
for (let i = 0; i < funcCount; i++) { | ||||||
if (funcIsJS[i] || funcResource[i] !== -1) { | ||||||
continue; | ||||||
} | ||||||
const locationString = stringTable.getString(funcName[i]); | ||||||
if (locationString.startsWith('0x')) { | ||||||
Comment on lines
+929
to
+933
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of reusing |
||||||
isUnaccountedNativeFunctionArr[i] = 1; | ||||||
} | ||||||
} | ||||||
return isUnaccountedNativeFunctionArr; | ||||||
} | ||||||
|
||||||
export function mergeUnaccountedNativeFunctions(thread: Thread): Thread { | ||||||
const isUnaccountedNativeFunctionArr = getUnaccountedNativeFunctions(thread); | ||||||
return mergeFunctions(thread, isUnaccountedNativeFunctionArr); | ||||||
} | ||||||
|
||||||
export function mergeFunctions( | ||||||
thread: Thread, | ||||||
shouldMergeFunction: Uint8Array | ||||||
): Thread { | ||||||
const { stackTable, frameTable } = thread; | ||||||
|
||||||
// A map oldStack -> newStack+1, implemented as a Uint32Array for performance. | ||||||
// If newStack+1 is zero it means "null", i.e. this stack was filtered out. | ||||||
// Typed arrays are initialized to zero, which we interpret as null. | ||||||
// | ||||||
// For each old stack, the new stack is computed as follows: | ||||||
// - If the old stack's function is not funcIndexToMerge, then the new stack | ||||||
// is the same as the old stack. | ||||||
// - If the old stack's function is funcIndexToMerge, then the new stack is | ||||||
// the closest ancestor whose func is not funcIndexToMerge, or null if no | ||||||
Comment on lines
+956
to
+959
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this comment needs to be updated to replace |
||||||
// such ancestor exists. | ||||||
// | ||||||
// We only compute a new prefix column; the other columns are copied from the | ||||||
// old stack table. The skipped stacks are "orphaned"; they'll still be present | ||||||
// in the new stack table but not referenced by samples or other stacks. | ||||||
const oldStackToNewStackPlusOne = new Uint32Array(stackTable.length); | ||||||
|
||||||
const stackTableFrameCol = stackTable.frame; | ||||||
const frameTableFuncCol = frameTable.func; | ||||||
const oldPrefixCol = stackTable.prefix; | ||||||
const newPrefixCol = new Array(stackTable.length); | ||||||
|
||||||
for (let stackIndex = 0; stackIndex < stackTable.length; stackIndex++) { | ||||||
const oldPrefix = oldPrefixCol[stackIndex]; | ||||||
const newPrefixPlusOne = | ||||||
oldPrefix === null ? 0 : oldStackToNewStackPlusOne[oldPrefix]; | ||||||
|
||||||
const frameIndex = stackTableFrameCol[stackIndex]; | ||||||
const funcIndex = frameTableFuncCol[frameIndex]; | ||||||
if (shouldMergeFunction[funcIndex] === 1) { | ||||||
oldStackToNewStackPlusOne[stackIndex] = newPrefixPlusOne; | ||||||
} else { | ||||||
oldStackToNewStackPlusOne[stackIndex] = stackIndex + 1; | ||||||
} | ||||||
const newPrefix = newPrefixPlusOne === 0 ? null : newPrefixPlusOne - 1; | ||||||
newPrefixCol[stackIndex] = newPrefix; | ||||||
} | ||||||
|
||||||
const newStackTable = { | ||||||
...stackTable, | ||||||
prefix: newPrefixCol, | ||||||
}; | ||||||
|
||||||
return updateThreadStacks(thread, newStackTable, (oldStack) => { | ||||||
if (oldStack === null) { | ||||||
return null; | ||||||
} | ||||||
const newStackPlusOne = oldStackToNewStackPlusOne[oldStack]; | ||||||
return newStackPlusOne === 0 ? null : newStackPlusOne - 1; | ||||||
}); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Drop any samples that contain the given function. | ||||||
*/ | ||||||
export function dropFunction( | ||||||
thread: Thread, | ||||||
funcIndexToDrop: IndexIntoFuncTable | ||||||
) { | ||||||
): Thread { | ||||||
const { stackTable, frameTable } = thread; | ||||||
|
||||||
// Go through each stack, and label it as containing the function or not. | ||||||
|
@@ -1774,6 +1914,8 @@ export function applyTransform( | |||||
); | ||||||
case 'merge-function': | ||||||
return mergeFunction(thread, transform.funcIndex); | ||||||
case 'merge-unaccounted-native-functions': | ||||||
return mergeUnaccountedNativeFunctions(thread); | ||||||
case 'drop-function': | ||||||
return dropFunction(thread, transform.funcIndex); | ||||||
case 'focus-function': | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the doc could also point to the other l10n key
CallNodeContextMenu--transform-merge-unaccounted-native-functions
? @flodolo is there a way to do it? or would you prefer that we copy the doc over there too?