-
Notifications
You must be signed in to change notification settings - Fork 254
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 support for Rust v0 symbol mangling scheme #491
Conversation
This is required because emscripten glue code now uses es6 features like logicalAssignment "??=" which is only supported by newer version of parcel.
package.json
Outdated
@@ -45,9 +45,10 @@ | |||
"jsverify": "0.8.3", | |||
"jszip": "3.1.5", | |||
"pako": "1.0.6", | |||
"parcel-bundler": "1.12.4", | |||
"parcel": "2.13.3", |
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.
Was this necessary as part of this change? I'm not opposed per-se, but I don't like upgrading dependencies as part of PRs that introduce other changes in case they need reverting
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.
Fully understand. I added the explanation in the commit. Sadly the emscripten generated code is a raw JavaScript file that contains features that are not supported by That version of parcel-bundled hence why I had to upgrade it.
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.
This is required because emscripten glue code now uses es6 features
like logicalAssignment "??=" which is only supported by newer version
of parcel.
src/lib/profile.ts
Outdated
if (!demangleCpp) { | ||
demangleCpp = (await demangleCppModule).demangleCpp | ||
// This function converts a mangled C++ and Rust name into a human-readable symbol. | ||
if (frame.name.startsWith('__Z') || frame.name.startsWith('_R') || frame.name.startsWith('_Z')) { |
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.
Hmmm is there any other way of sanely auto-detecting this? __Z
is so rare as a prefix it seems unlikely to yield false positives, but _R
and _Z
seem far more likely and I'd like to avoid incurring this loading cost unless needed
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.
I don't see any other way sadly.
Unless mistaken, _Z is the default mangling prefix and __Z happens mostly on macOS where symbols get an extra _.
I tested with a raw undemangled C++ perf trace on Linux and speedscope didn't demsngle it. I added this as a result but should have made a separate commit to explain.
Referencing rust-lang/rust#60705 for cross visibility :) |
frame.name.startsWith('_Z') | ||
) { | ||
if (!demangle) { | ||
const demangleModule = await import('./demangle') |
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.
The import
was above intentionally at the top of the file to cause eager, non-blocking loading of this module. As written here, the load of this module will only begin after a profile import, which will be much slower.
In this case, I think we want both the import
and the loadDemangling
to happen eagerly, and then to await the promise in here.
I think the right way of doing that is to eagerly do the import
here, and to eagerly create the wasm module in demangle.ts
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.
From what I read, demangle is already imported eagerly by application.tsx
no ?
As for demangle.ts
, is it fine if I do this ?
diff --git a/src/lib/demangle/demangle.ts b/src/lib/demangle/demangle.ts
index b69adeb..70f17a0 100644
--- a/src/lib/demangle/demangle.ts
+++ b/src/lib/demangle/demangle.ts
@@ -1,11 +1,13 @@
import createWasmDemangleModule from './demangle.wasm'
+const wasmDemangleModulePromise = createWasmDemangleModule().then((module) => module)
+
const cache = new Map<string, string>()
export async function loadDemangling(): Promise<(name: string) => string> {
// This function converts a mangled C++ name such as "__ZNK7Support6ColorFeqERKS0_"
// into a human-readable symbol (in this case "Support::ColorF::==(Support::ColorF&)")
- const wasmDemangleModule = await createWasmDemangleModule()
+ const wasmDemangleModule = await wasmDemangleModulePromise
return cached(wasmDemangleModule.wasm_demangle)
}
That way, everything is eager from application.tsx
.
import ("./demangle")
from profile.tsx
is awaiting the eager loaded import from application.tsx
and loadDemangling
in profile.tsx
just await the eager loaded wasmModule triggered by the eager loding of demangle
from `application.tsx.
It looks like there's a bunch of stuff downstream of the parcel upgrade that's otherwise unrelated to this change. I'm going to fix it, then ask you to rebase this on top of that |
@jlfwong I have an idea that might save us from upgrade parcel. Basically babelize the generated emscripten code so that it's compatible with whatever javascript version is compatibel with parce-bundler at 1.12.4. Either from the Makefile directly or as part of the bundling. I'll make a test before suggesting a change here. |
@cerisier that would be appreciated! I'll need to deal with the bundling problems eventually regardless, but would love to land your changes without dealing with that first. Rather than babel to do this, I might suggest using Something like...
|
Wonderful I'll do that |
@jlfwong done. I reverted your PR about upgrading parcel in the mean time. Let me know if you want to do it on main first and I'll just re-revert it here. |
@cerisier Thanks -- I reverted in main. Can you rebase please to make this PR diff easier to read? |
In the process of trying to fix this problem a different way by switching to esbuild, I realized ran into another challenging requirement: the ability to load things off of This hasn't been an issue to date because speedscope has used neither wasm nor Switching to esbuild necessitates module scripts for code splitting, but I can work around it by not using code splitting for the local I think this is all resolvable, but a bit of added pain. See #295 for more detail EDIT: Skimming through the code in this PR, it looks like the wasm file contents are already base64 encoded in here. So, false alarm! |
@jlfwong done |
Prettier fixed |
Sorry for all the flux -- I landed the change to migrate to esbuild and deployed everything. Can you try rebasing one more time and make sure everything looks good? You can also now remove the steps needed to cross-compile with esbuild to get rid of the |
@jlfwong done ! |
✅ |
Thanks! Will deploy later today |
@cerisier How do I easily create a profile which has symbols mangled in this way? I'd like to test locally before I deploy this to make sure it's working in all the various environments speedscope runs in |
I deleted the example trace I was testing this with :( |
@cerisier Thanks! Was able to test it, everything looks good, and this is now deployed on https://www.speedscope.app/ and on npm as |
Attempt to tackle #474 as per the discussion in the issue thread. Fixes #474.
Until now, demangling was handled by shipping emscripten compiled javascript as a blob and the exact details of how it was compiled had been partially lost, only that it used part of https://github.com/nattofriends/c-filtjs.
In order to support Rust demangling, it has been discussed that we'd take the opportunity to include the source code as part of speedscope and document the process.
This PR follows several guidelines:
c-filtjs
was using (GNU libiberty
fromgcc
which is written in pure C).The approach I took is to write a wrapper
demangle
function in C which routes demangling to the proper demangler.It uses
cplus_demangle_v3
for theItanium C++ ABI
andrust_demangle
for Rust.I followed the same logic as
llvm::demangle
for routing.This file is then compiled by
emscripten
into a single JavaScript file embedding the.wasm
file.Improvements
While working on this, I realized that the implementation within
llvm
support more cases.I learned about the existence of symbols with up to 4
_
like____Z
which are not handled bylibiberty
directly.One improvement could be to use
llvm::demangle
rather thanGNU libiberty
and support more cases.Swift demangling
Swift
also has its own mangling scheme and while profilers likeInstruments
already demangle Swift symbols on macOS, this support is inexistant on Linux. Adn sinceSwift
is being used more and more outside of macOS, it is likely that we'll start to see perf traces of Swift programs recorded on Linux which wouldn't get demangling directly.