Closed
Bug 968510
Opened 11 years ago
Closed 9 years ago
Add support for demangling C symbol names in the Firefox profiler?
Categories
(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P3)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(firefox46 fixed)
RESOLVED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: jujjyl, Assigned: jsantell)
References
Details
(Whiteboard: [devtools-platform][gaming-tools])
Attachments
(3 files)
Consider the following Emscripten-compiled application run in the profiler:
http://clb.demon.fi/dump/c_callstack.png
It would be useful to have a checkbox that, when enabled, would show the demangled names of the C symbols in the tree. This would allow easy toggling of callstack versions and locating the appropriate places in code.
You can visit the page that outputs the above profile at
https://dl.dropboxusercontent.com/u/40949268/emcc/10kCubes_benchmark/10kCubes.html?--benchmark&--objects&1000
The C callstacks in Emscripten follow Clang C++ mangling rules, which are quite standard. Alon Zakai has already implemented in the Emscripten repository at
https://github.com/kripken/emscripten/blob/master/src/preamble.js#L681
a JS version of function demangle(mangledSymbolString) that returns a demangled string of the given function. That could be extracted to a separate file demangle.js for easier co-maintenance between Firefox profiler and the Emscripten repo.
Reporter | ||
Comment 1•11 years ago
|
||
Sorry, mixed up the screenshot and the assigned category between geckoprofiler.xpi and the built-in profiler. Naturally the feature would equally apply to both tools.
Comment 2•11 years ago
|
||
We wanted to do this but we didn't have a library for doing this. I'll look into it.
Updated•10 years ago
|
Priority: -- → P3
Comment 3•10 years ago
|
||
For a library, can use libcxxabi, that's how emscripten does stack traces. If you want, I can provide a JS file that does it. It won't be tiny, though, demanging code is surprisingly large (thousands of lines of LLVM IR, I didn't measure JS). Another option is emscripten's mini-demangler,
https://github.com/kripken/emscripten/blob/master/src/preamble.js#L780
which is handwritten and can demangle "common" stuff, but not everything. Emscripten uses the mini-demangler by default, we include libcxxabi's only when requested, due to code size.
Reporter | ||
Comment 5•10 years ago
|
||
To clarify, this bug report deals with demangling C++ mangling rules for functions, e.g. if I did a JS function with name
function _ZN9MainClass12ProcessFrameEv() {
// ...;
}
we are asking in this bug that the function name is recognized to be a mangled name under C++ compilation, and it would be shown as function "MainClass::ProcessFrame()" instead. (perhaps with an optional checkbox to toggle demangling on and off)
From the title in bug 1049853, that bug sounds like finding mappings from unknown (JIT-compiled?) stack traces which would otherwise show up as a raw address "0x12345678" back to the actual name of the function?
Comment 8•10 years ago
|
||
(In reply to Jukka Jylänki from comment #5)
You're right. There's bug 1132529 filed for that specifically, and all of these dupes (bug 1049853, bug 1111823) should've been of that one.
Updated•9 years ago
|
Whiteboard: [devtools-platform]
Assignee | ||
Updated•9 years ago
|
Blocks: perf-gaming
Whiteboard: [devtools-platform] → [devtools-platform][gaming-tools]
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 9•9 years ago
|
||
I've got a quick patch using emscripten's quick demangler (not the libcxxxabi one), and certainly has some problems with some function names, but usually they're atleast readable. I would think these are *good enough* to get the right idea, and we can further refine it if needed. They still link to, and show on hover, the mangled name used in JS.
So my questions: is this level of demangling sufficient? Are there other mangled formats we could expect, from Unity or Unreal for example, or another JS-as-a-compilation-target build process?
Flags: needinfo?(jujjyl)
Flags: needinfo?(azakai)
Assignee | ||
Comment 10•9 years ago
|
||
Also for simplicity, I removed the variant where the passed in mangled name is a number type, as that used some of emscripten's build magic in the file -- is that a variation we could expect? (I've never seen it while profiling emscripten AFAIK)
Comment 11•9 years ago
|
||
Yes, this should be sufficient, mangled names are standard (everywhere relevant that I can think of) and we'd get the same ones from Unity, Unreal, etc. As for the quick mini-demangler, we could improve it if necessary. But optimal might be the compiled complete one, the only downside there is it would be large. Is size an issue?
No need to handle an input number, that's emscripten magic stuff, yeah.
Flags: needinfo?(azakai)
Assignee | ||
Comment 12•9 years ago
|
||
How big is the compiled complete one? Maybe can use the simple one in release/beta, and the compiled one for aurora/nightly due to size. Another possibility maybe is, if it's really large, offer it as an addon, and the tool can check the existence for it and use the addon's demangler (which would be the compiled complete one).
Comment 13•9 years ago
|
||
Looks like 227K for the full one. That's from 5,000 lines of C++.
Assignee | ||
Comment 14•9 years ago
|
||
Can you link the full version? There are other tools that could benefit from this maybe (debugger showing the demangled name in a tooltip or something?) so maybe this could just be included.
Just posted on the mailing list[0] to get thoughts on the filesize vs including it optionally.
[0] https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/C3aTxnOEaJQ
Comment 15•9 years ago
|
||
Ok, I made a repo for the full version now,
https://github.com/kripken/cxx_demangle
demangle.js there provides demangle().
The size is 263K, after I made a benchmark and noticed that the previous 227K one was quite slow. The current one can do 20,000 demanglings in a second.
Assignee | ||
Comment 16•9 years ago
|
||
Looks pretty good! Still waiting to hear back from others regarding large files in m-c.
Some notes:
* In the simple demangler, if string does start with "__Z", we just return the original string name so all function names were passed through the demangler. In the cxx-demangler, if invalid, we get something empty, so we'll need to check before hand (is "__Z" sufficient?), as well as fall back to the original function name if could not demangle (so if there were a non mangled function name that started with __Z or any other function name)
* Need to build with common JS style `module.exports`
* Module.print is not defined -- stubbed this out
The build process I can change, but looks like the C++ source is not included to build, but if the Module.print stuff is removed, I could add the common JS wrappers/any stubs in the before.js/after.js no problem.
Thanks Alon!
Flags: needinfo?(jujjyl)
Comment 17•9 years ago
|
||
Yes, __Z is sufficient - probably faster anyhow to not call demangle() for anything not starting with that.
There isn't a C++ source in that repo, since the demangling code arrives from an Emscripten system library.
A PR with common JS exports etc. would be great. I really should know more about that stuff...
Screenshot looks great! Makes a huge difference I think for C++ devs to see the demangled names.
Reporter | ||
Comment 18•9 years ago
|
||
Whoa Jordan, this is amazing! Can't wait to have the tool available, looks much clearer.
Can it have a checkbox to toggle the demangling on/off? That would be useful when copypasting text out from the profiler.
I feel that it might be confusing if we used the lightweight handwritten version of stable Firefox, and the compiled demangler for Devedition/Nightly? (double the surface area for bugs, and difficult/silly to document to developer) If size ends up being a concern, perhaps we should just always use the simple one, and start improving it so that it is more complete with demangling all LLVM names?
Assignee | ||
Comment 19•9 years ago
|
||
I think we're going to land it as is. If size is a concern we can deal with that in the future.
Toggling's a good idea! Wrestling with some widget tests now, I think that'll be a follow up that we'll have on by default.
Assignee | ||
Comment 20•9 years ago
|
||
Some notes:
* The flame graph tests rely on ordering of the block data which is based on the frame key I guess? So the order has changed.
* `shouldDemangle` should ideally be on the demangle module, but due to how this is imported, this would be annoying to merge in upstream. Can add to the node module once we have node module loading (Bug 1231453). This is fine for now IMO
* Created bug 1232460 for the follow up on making this a toggleable option
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b6ec7c6a41f
Attachment #8698207 -
Flags: review?(vporof)
Comment 21•9 years ago
|
||
Comment on attachment 8698207 [details] [diff] [review]
968510-demangling.patch
Review of attachment 8698207 [details] [diff] [review]:
-----------------------------------------------------------------
::: devtools/client/shared/demangle.js
@@ +30,5 @@
> +var ra=[Sc,qc];return{_malloc:vc,_i64Subtract:Cc,_free:wc,_i64Add:Gc,_memmove:Hc,_memset:Bc,___cxa_demangle:Ba,_memcpy:Fc,_bitshift64Lshr:Dc,_bitshift64Shl:Ec,runPostSets:Ac,stackAlloc:sa,stackSave:ta,stackRestore:ua,establishStackSpace:va,setThrew:wa,setTempRet0:za,getTempRet0:Aa,dynCall_iiii:Rc}})
> +
> +
> +// EMSCRIPTEN_END_ASM
> +(Module.asmGlobalArg,Module.asmLibraryArg,buffer);var ___cxa_demangle=Module["___cxa_demangle"]=asm["___cxa_demangle"];var _i64Subtract=Module["_i64Subtract"]=asm["_i64Subtract"];var _free=Module["_free"]=asm["_free"];var runPostSets=Module["runPostSets"]=asm["runPostSets"];var _i64Add=Module["_i64Add"]=asm["_i64Add"];var _memmove=Module["_memmove"]=asm["_memmove"];var _memset=Module["_memset"]=asm["_memset"];var _malloc=Module["_malloc"]=asm["_malloc"];var _memcpy=Module["_memcpy"]=asm["_memcpy"];var _bitshift64Lshr=Module["_bitshift64Lshr"]=asm["_bitshift64Lshr"];var _bitshift64Shl=Module["_bitshift64Shl"]=asm["_bitshift64Shl"];var dynCall_iiii=Module["dynCall_iiii"]=asm["dynCall_iiii"];Runtime.stackAlloc=asm["stackAlloc"];Runtime.stackSave=asm["stackSave"];Runtime.stackRestore=asm["stackRestore"];Runtime.establishStackSpace=asm["establishStackSpace"];Runtime.setTempRet0=asm["setTempRet0"];Runtime.getTempRet0=asm["getTempRet0"];function ExitStatus(status){this.name="ExitStatus";this.message="Program terminated with exit("+status+")";this.status=status}ExitStatus.prototype=new Error;ExitStatus.prototype.constructor=ExitStatus;var initialStackTop;var preloadStartTime=null;var calledMain=false;dependenciesFulfilled=function runCaller(){if(!Module["calledRun"])run();if(!Module["calledRun"])dependenciesFulfilled=runCaller};Module["callMain"]=Module.callMain=function callMain(args){assert(runDependencies==0,"cannot call main when async dependencies remain! (listen on __ATMAIN__)");assert(__ATPRERUN__.length==0,"cannot call main when preRun functions remain to be called");args=args||[];ensureInitRuntime();var argc=args.length+1;function pad(){for(var i=0;i<4-1;i++){argv.push(0)}}var argv=[allocate(intArrayFromString(Module["thisProgram"]),"i8",ALLOC_NORMAL)];pad();for(var i=0;i<argc-1;i=i+1){argv.push(allocate(intArrayFromString(args[i]),"i8",ALLOC_NORMAL));pad()}argv.push(0);argv=allocate(argv,"i32",ALLOC_NORMAL);try{var ret=Module["_main"](argc,argv,0);exit(ret,true)}catch(e){if(e instanceof ExitStatus){return}else if(e=="SimulateInfiniteLoop"){Module["noExitRuntime"]=true;return}else{if(e&&typeof e==="object"&&e.stack)Module.printErr("exception thrown: "+[e,e.stack]);throw e}}finally{calledMain=true}};function run(args){args=args||Module["arguments"];if(preloadStartTime===null)preloadStartTime=Date.now();if(runDependencies>0){return}preRun();if(runDependencies>0)return;if(Module["calledRun"])return;function doRun(){if(Module["calledRun"])return;Module["calledRun"]=true;if(ABORT)return;ensureInitRuntime();preMain();if(Module["onRuntimeInitialized"])Module["onRuntimeInitialized"]();if(Module["_main"]&&shouldRunNow)Module["callMain"](args);postRun()}if(Module["setStatus"]){Module["setStatus"]("Running...");setTimeout((function(){setTimeout((function(){Module["setStatus"]("")}),1);doRun()}),1)}else{doRun()}}Module["run"]=Module.run=run;function exit(status,implicit){if(implicit&&Module["noExitRuntime"]){return}if(Module["noExitRuntime"]){}else{ABORT=true;EXITSTATUS=status;STACKTOP=initialStackTop;exitRuntime();if(Module["onExit"])Module["onExit"](status)}if(ENVIRONMENT_IS_NODE){process["stdout"]["once"]("drain",(function(){process["exit"](status)}));console.log(" ");setTimeout((function(){process["exit"](status)}),500)}else if(ENVIRONMENT_IS_SHELL&&typeof quit==="function"){quit(status)}throw new ExitStatus(status)}Module["exit"]=Module.exit=exit;var abortDecorators=[];function abort(what){if(what!==undefined){Module.print(what);Module.printErr(what);what=JSON.stringify(what)}else{what=""}ABORT=true;EXITSTATUS=1;var extra="\nIf this abort() is unexpected, build with -s ASSERTIONS=1 which can give more information.";var output="abort("+what+") at "+stackTrace()+extra;if(abortDecorators){abortDecorators.forEach((function(decorator){output=decorator(output,what)}))}throw output}Module["abort"]=Module.abort=abort;if(Module["preInit"]){if(typeof Module["preInit"]=="function")Module["preInit"]=[Module["preInit"]];while(Module["preInit"].length>0){Module["preInit"].pop()()}}var shouldRunNow=true;if(Module["noInitialRun"]){shouldRunNow=false}run()
wat
Attachment #8698207 -
Flags: review?(vporof) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
Hi, this failed to apply:
applying 968510-demangling.patch
patching file devtools/client/performance/test/browser_profiler_tree-view-02.js
Hunk #2 FAILED at 91
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/performance/test/browser_profiler_tree-view-02.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 968510-demangling.patch
Flags: needinfo?(jsantell)
Keywords: checkin-needed
Assignee | ||
Comment 23•9 years ago
|
||
Had to resolve after bug 1081245 -- all fixed and landed
Flags: needinfo?(jsantell)
Comment 25•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment 26•9 years ago
|
||
[bugday-20160323]
not for beginners. Profiler debugging knowledge required.
Status: RESOLVED,FIXED --> UNVERIFIED
Component:
Name Firefox
Version 46.0b2
Build ID 20160314144540
Update Channel beta
User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS Windows 7 SP1 x86_64
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•