Skip to content
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

Merged
merged 21 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
node_modules
.cache
.parcel-cache
dist
.idea
coverage
Expand Down
4 changes: 2 additions & 2 deletions assets/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@
</head>

<body>
<script src="../src/speedscope.tsx"></script>
<script type="module" src="../src/speedscope.tsx"></script>
</body>

</html>
</html>
42,810 changes: 22,572 additions & 20,238 deletions package-lock.json

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Owner

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

"preact": "10.4.1",
"prettier": "3.1.1",
"process": "^0.11.10",
"protobufjs": "6.8.8",
"source-map": "0.6.1",
"ts-jest": "24.3.0",
Expand Down
2 changes: 2 additions & 0 deletions src/gl/graphics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

// NOTE: This file intentionally has no dependencies.

declare const process: any;

Check failure on line 28 in src/gl/graphics.ts

View workflow job for this annotation

GitHub Actions / test (18.x)

Delete `;`

Check failure on line 28 in src/gl/graphics.ts

View workflow job for this annotation

GitHub Actions / test (20.x)

Delete `;`

// Dependencies & polyfills for import from skew
const RELEASE =
typeof process !== 'undefined' && process.env && process.env.NODE_ENV === 'production'
Expand Down
13 changes: 0 additions & 13 deletions src/lib/demangle-cpp.test.ts

This file was deleted.

32 changes: 0 additions & 32 deletions src/lib/demangle-cpp.ts

This file was deleted.

23 changes: 23 additions & 0 deletions src/lib/demangle.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import {loadDemangling} from './demangle'

test('demangle', async () => {
const demangle = await loadDemangling()

expect(demangle('a')).toBe('a')
expect(demangle('someUnobfuscatedFunction')).toBe('someUnobfuscatedFunction')

// C++ mangling
expect(demangle('__ZNK7Support6ColorFeqERKS0_')).toBe(
'Support::ColorF::operator==(Support::ColorF const&) const',
)
// Running a second time to test the cache
expect(demangle('__ZNK7Support6ColorFeqERKS0_')).toBe(
'Support::ColorF::operator==(Support::ColorF const&) const',
)

// Rust v0 mangling
expect(demangle('_RNvCskwGfYPst2Cb_3foo16example_function')).toBe('foo::example_function')

// Rust legacy mangling
expect(demangle('_ZN3std2fs8Metadata7created17h8df207f105c5d474E')).toBe('std::fs::Metadata::created::h8df207f105c5d474')

Check failure on line 22 in src/lib/demangle.test.ts

View workflow job for this annotation

GitHub Actions / test (18.x)

Replace `'std::fs::Metadata::created::h8df207f105c5d474'` with `⏎····'std::fs::Metadata::created::h8df207f105c5d474',⏎··`

Check failure on line 22 in src/lib/demangle.test.ts

View workflow job for this annotation

GitHub Actions / test (20.x)

Replace `'std::fs::Metadata::created::h8df207f105c5d474'` with `⏎····'std::fs::Metadata::created::h8df207f105c5d474',⏎··`
})
25 changes: 25 additions & 0 deletions src/lib/demangle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import createWasmDemangleModule from "./demangle.wasm";

Check failure on line 1 in src/lib/demangle.ts

View workflow job for this annotation

GitHub Actions / test (18.x)

Replace `"./demangle.wasm";` with `'./demangle.wasm'`

Check failure on line 1 in src/lib/demangle.ts

View workflow job for this annotation

GitHub Actions / test (20.x)

Replace `"./demangle.wasm";` with `'./demangle.wasm'`

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();

Check failure on line 8 in src/lib/demangle.ts

View workflow job for this annotation

GitHub Actions / test (18.x)

Delete `;`

Check failure on line 8 in src/lib/demangle.ts

View workflow job for this annotation

GitHub Actions / test (20.x)

Delete `;`
return cached(wasmDemangleModule.wasm_demangle);

Check failure on line 9 in src/lib/demangle.ts

View workflow job for this annotation

GitHub Actions / test (18.x)

Delete `;`

Check failure on line 9 in src/lib/demangle.ts

View workflow job for this annotation

GitHub Actions / test (20.x)

Delete `;`
}

function cached(demangle: (name: string) => string): (name: string) => string {
return (name: string): string => {
let result = cache.get(name)
if (result !== undefined) {
name = result
} else {
result = demangle(name)
result = result === '' ? name : result
cache.set(name, result)
name = result
}
return name
}
}
5 changes: 5 additions & 0 deletions src/lib/demangle.wasm.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
interface WasmDemangleModule {
wasm_demangle(mangled: string): string

Check failure on line 2 in src/lib/demangle.wasm.d.ts

View workflow job for this annotation

GitHub Actions / test (18.x)

Delete `··`

Check failure on line 2 in src/lib/demangle.wasm.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x)

Delete `··`
}

export default function ModuleFactory(options?: unknown): Promise<WasmDemangleModule>;

Check failure on line 5 in src/lib/demangle.wasm.d.ts

View workflow job for this annotation

GitHub Actions / test (18.x)

Delete `;`

Check failure on line 5 in src/lib/demangle.wasm.d.ts

View workflow job for this annotation

GitHub Actions / test (20.x)

Delete `;`
22 changes: 22 additions & 0 deletions src/lib/demangle.wasm.js

Large diffs are not rendered by default.

15 changes: 7 additions & 8 deletions src/lib/profile.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {lastOf, KeyedSet} from './utils'
import {ValueFormatter, RawValueFormatter} from './value-formatters'
import {FileFormat} from './file-format-spec'
const demangleCppModule = import('./demangle-cpp')

export interface FrameInfo {
key: string | number
Expand Down Expand Up @@ -404,16 +403,16 @@

// Demangle symbols for readability
async demangle() {
let demangleCpp: ((name: string) => string) | null = null
let demangle: ((name: string) => string) | null = null

for (let frame of this.frames) {
// This function converts a mangled C++ name such as "__ZNK7Support6ColorFeqERKS0_"
// into a human-readable symbol (in this case "Support::ColorF::==(Support::ColorF&)")
if (frame.name.startsWith('__Z')) {
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')) {

Check failure on line 410 in src/lib/profile.ts

View workflow job for this annotation

GitHub Actions / test (18.x)

Replace `frame.name.startsWith('__Z')·||·frame.name.startsWith('_R')·||·frame.name.startsWith('_Z')` with `⏎········frame.name.startsWith('__Z')·||⏎········frame.name.startsWith('_R')·||⏎········frame.name.startsWith('_Z')⏎······`

Check failure on line 410 in src/lib/profile.ts

View workflow job for this annotation

GitHub Actions / test (20.x)

Replace `frame.name.startsWith('__Z')·||·frame.name.startsWith('_R')·||·frame.name.startsWith('_Z')` with `⏎········frame.name.startsWith('__Z')·||⏎········frame.name.startsWith('_R')·||⏎········frame.name.startsWith('_Z')⏎······`
Copy link
Owner

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

Copy link
Contributor Author

@cerisier cerisier Jan 12, 2025

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.

if (!demangle) {
const demangleModule = await import('./demangle')
Copy link
Owner

@jlfwong jlfwong Jan 13, 2025

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

Copy link
Contributor Author

@cerisier cerisier Jan 13, 2025

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.

demangle = await demangleModule.loadDemangling()
}
frame.name = demangleCpp(frame.name)
frame.name = demangle(frame.name)
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions src/views/application.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
// We put them all in one place so we can directly control the relative priority
// of these.
importModule.then(() => {})
import('../lib/demangle-cpp').then(() => {})
import('../lib/demangle').then(() => {})
import('source-map').then(() => {})

async function importProfilesFromText(
Expand Down Expand Up @@ -56,8 +56,7 @@
return (await importModule).importFromFileSystemDirectoryEntry(entry)
}

declare function require(x: string): any
const exampleProfileURL = require('../../sample/profiles/stackcollapse/perf-vertx-stacks-01-collapsed-all.txt')
const exampleProfileURL = new URL('../../sample/profiles/stackcollapse/perf-vertx-stacks-01-collapsed-all.txt', import.meta.url)

Check failure on line 59 in src/views/application.tsx

View workflow job for this annotation

GitHub Actions / test (18.x)

Replace `'../../sample/profiles/stackcollapse/perf-vertx-stacks-01-collapsed-all.txt',·import.meta.url` with `⏎··'../../sample/profiles/stackcollapse/perf-vertx-stacks-01-collapsed-all.txt',⏎··import.meta.url,⏎`

Check failure on line 59 in src/views/application.tsx

View workflow job for this annotation

GitHub Actions / test (20.x)

Replace `'../../sample/profiles/stackcollapse/perf-vertx-stacks-01-collapsed-all.txt',·import.meta.url` with `⏎··'../../sample/profiles/stackcollapse/perf-vertx-stacks-01-collapsed-all.txt',⏎··import.meta.url,⏎`

function isFileSystemDirectoryEntry(entry: FileSystemEntry): entry is FileSystemDirectoryEntry {
return entry != null && entry.isDirectory
Expand Down
Loading