Add a "Merge unaccounted native frames" transform #5141
Conversation
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.
|
What's the status of this PR? |
|
Sorry, the status is that I need to address the review comments. One of julien's review comments is about adding another comment inside the app.ftl file. Actually, let me do that now and then it's unblocked for your review. |
mstange
force-pushed
the
merge-unaccounted-native-functions-transform
branch
from
e928146 to
fe3ba38
Compare
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.
mstange
force-pushed
the
merge-unaccounted-native-functions-transform
branch
from
fe3ba38 to
0b75184
Compare
|
@flodolo This PR is now ready for your review. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5141 +/- ##
==========================================
- Coverage 88.62% 88.61% -0.01%
==========================================
Files 308 308
Lines 28060 28129 +69
Branches 7599 7619 +20
==========================================
+ Hits 24868 24927 +59
- Misses 2978 2988 +10
Partials 214 214 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| .title = | ||
| Removes stack frames such as unsymbolicated JIT frames from the call tree, | ||
| and make their callers absorb their cost. | ||
| Specifically, this transform merges any native stack frames which were |
There was a problem hiding this comment.
I'm a bit lost on the second part (mind you, not a developer here).
Is this transform merging unaccounted for frames to a shared library (the action is "merge to a shared library"), or is this merging frames not belonging to a shared library ("merge specific frames, i.e. those that don't belong to a library")? Also, what does unaccounted for mean here: not examined, not included, 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.