Add a "Merge unaccounted native frames" transform #5141
Conversation
This makes it easier to analyze profiles with JIT frames. This transform gets rid of 0x12345 frames in the call tree - but only those that haven't been accounted to a shared library, i.e. only frames which we consider to be "probably JIT" frames. This transform does not merge unsymbolicated frames for which we know what library they belong to. Ideally, we wouldn't have those stray hex addresses in the profiles; either Firefox would filter them out during stack merging, or it should give us the correct memory ranges for JIT functions so that we can account these addresses to the right JS function / JIT trampoline etc. But it's not doing that correctly today. https://bugzilla.mozilla.org/show_bug.cgi?id=1463559 is one of the bugs about this. For now it seems easier to just offer a transform which gets rid of these stray JIT address frames. This new transform doesn't really clutter anything up; users will only know about it if they right-click such a frame / call node. Even if Firefox starts giving us better information for JIT frames, this transform can still be useful. For example, you could see this type of stack frame in profiles that are gathered with samply, on applications that use other JITs or applications which don't have Jitdump instrumentation enabled. Another use case would be if Firefox uses a system library that makes use of jitting, for example a software OpenGL implementation. Those libraries can't be expected to expose their JIT details to our profiler.
There was a problem hiding this comment.
This looks good to me, thanks!
1 thing need to be fixed before landing: implement the shortcut key
Then there's some nitpicking over which shortcut key to use or how to serialize to the URL.
And here and there some comments changes, and some minor refactoring.
| @@ -1081,6 +1087,10 @@ TransformNavigator--merge-call-node = Merge Node: { $item } | |||
| # $item (String) - Name of the function that transform applied to. | |||
| TransformNavigator--merge-function = Merge: { $item } | |||
|
|
|||
| # "Merge unaccounted native frames" transform. | |||
| # See: https://profiler.firefox.com/docs/#/./guide-filtering-call-trees?id=merge | |||
There was a problem hiding this comment.
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?
| ? this.renderTransformMenuItem({ | ||
| l10nId: | ||
| 'CallNodeContextMenu--transform-merge-unaccounted-native-functions', | ||
| shortcut: 'J', |
There was a problem hiding this comment.
you need to handle this shortcut to handleCallNodeTransformShortcut in actions/profile-view
May I ask why J? I think I'd use "u" or "U" for "unaccounted"?
I guess this may be for "JIT" but because it's not mentioned anywhere it could be difficult to remember.
| @@ -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.
(optional) not a super big deal, but what about simply mu or mun?
| @@ -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.
nit
| * result[funcIndex] === 1 iff isUnaccountedNativeFunction(funcIndex) | |
| * result[funcIndex] === 1 if isUnaccountedNativeFunction(funcIndex) |
There was a problem hiding this comment.
(I think iff (if and only if) makes sense here)
There was a problem hiding this comment.
oh yeah okay, got it now. All my maths were done in french so I didn't think of that :-)
| if (funcIsJS[i] || funcResource[i] !== -1) { | ||
| continue; | ||
| } | ||
| const locationString = stringTable.getString(funcName[i]); | ||
| if (locationString.startsWith('0x')) { |
There was a problem hiding this comment.
What do you think of reusing isUnaccountedNativeFunction?
| // - 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 |
There was a problem hiding this comment.
nit: this comment needs to be updated to replace funcIndexToMerge with something else
Production | Deploy preview
This makes it easier to analyze profiles with JIT frames.
This transform gets rid of 0x12345 frames in the call tree - but only those that haven't been accounted to a shared library, i.e. only frames which we consider to be "probably JIT" frames. This transform does not merge unsymbolicated frames for which we know what library they belong to.
Ideally, we wouldn't have those stray hex addresses in the profiles; either Firefox would filter them out during stack merging, or it should give us the correct memory ranges for JIT functions so that we can account these addresses to the right JS function / JIT trampoline etc. But it's not doing that correctly today.
https://bugzilla.mozilla.org/show_bug.cgi?id=1463559 is one of the bugs about this.
For now it seems easier to just offer a transform which gets rid of these stray JIT address frames. This new transform doesn't really clutter anything up; users will only know about it if they right-click such a frame / call node.
Even if Firefox starts giving us better information for JIT frames, this transform can still be useful. For example, you could see this type of stack frame in profiles that are gathered with samply, on applications that use other JITs or applications which don't have Jitdump instrumentation enabled.
Another use case would be if Firefox uses a system library that makes use of jitting, for example a software OpenGL implementation. Those libraries can't be expected to expose their JIT details to our profiler.