-
Notifications
You must be signed in to change notification settings - Fork 301
Basic Java Flight Recorder support #533
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
Conversation
jlfwong
left a comment
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.
Thanks for this @beatbrot!
Can you say more about what makes the test setup cumbersome here? I'm generally reluctant to have format support that is just wholly untested.
I needed quite a bit of setup to get this working. I am wondering whether this is worth the effort...
|
@jlfwong So I added the necessary test infrastructure.... I think it is quite cumbersome. If you have any idea on how to simplify this, please tell me! Other than that, it is your call whether we want to introduce this complexity. |
|
I think I'm not fully following what babel is needed for here, or why the wasm import pathway needs to be so different for in-browser and node via jest As a first step of simplification, it should be possible to use esbuild for transpilation instead of babel here via https://github.com/aelbore/esbuild-jest or similar, which should hopefully enable more sharing of configuration (it might actually be the right fix to replace ts-jest with esbuild-jest too) But even given that, it seems like I'm missing something about why the wasm dependency requires a babel transform For example, we have pre-existing tests which depend on wasm component being loaded in jest: https://github.com/jlfwong/speedscope/blob/main/src/lib/demangle/demangle.test.ts |
|
@jlfwong Sorry for the lacking explanation on my side :) I'll explain what I had to do:
So jfrview is my Rust project and I used wasm-pack to generate Javascript bindings and bundle it as NPM dependency. WASM-Pack asked me what kind of target I have and I answered "browser". Due to this, wasm-pack emitted ESM Modules which are not natively understood by jest. It seems like I am the first dependency doing this which is why I needed babel to translate ESM Modules to CommonJS. The generated javascript bindings also use the
Yes, the demangle.wasm module uses emscripten to generate WASM from C code. The bindings work fundamentally different: Emscripten emits CommonJS modules and inlines the WASM bytecode into the js file (see here). This inlining however kills lazy loading. The wasm-pack bindings instead offer an This lazy loading is also why I need the different import pathways. In the Browser, I want to do a fetch to load the WASM from the server. In Jest, there is no server. Instead I need to load the WASM from the file system. I will try esbuild-jest and see if it alleviates some problems. I'll get back to you after I find out :) All in all, I have to say that the Javascript ecosystem really seems to be rather messy. I love your project which is why I will try my best to get this PR ready. But it is really frustrating to me that I have to do so much configuration and transpiling for something which seems straightforward from the outside. |
No argument here :) Appreciate you working through some of the tangled pain |
|
@jlfwong I managed to simplify the code quite a bit! Instead of doing the jest/prod switch in the production code, I use a jest mock for it. I also managed to use esbuild-jest for transformation of JS files. It unfortunately did not work for the ts files. To satisfy the typescript compiler, I had to mock an API that is used by jfrview's typescript definitions. This is imo the only ugly bit left :/ What do you think? Would this be a mergeable state for you? |
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.
Pull request overview
This PR adds support for importing Java Flight Recorder (JFR) profiles, a profiling format used by Java applications. The implementation uses the jfrview WASM library to parse JFR files and creates two profile views: one with native calls included and one without.
Key Changes:
- Implements JFR file parsing using the jfrview WASM library
- Adds file extension (.jfr) and magic byte detection for JFR files
- Configures the build system and test environment to support WASM modules
Reviewed changes
Copilot reviewed 7 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/import/java-flight-recorder.ts | Main implementation for importing JFR profiles with native/non-native call filtering |
| src/import/java-flight-recorder.test.ts | Test case for JFR profile import |
| src/import/java-flight-record.mock.ts | Mock for loading WASM binary in test environment |
| src/import/index.ts | Integration of JFR importer into main import flow with file extension and magic byte detection |
| src/global.d.ts | TypeScript type definitions for Symbol.dispose used by jfrview |
| src/jest-setup.js | Added TextEncoder global for test environment |
| scripts/esbuild-shared.ts | Added .wasm file loader support for bundling |
| package.json | Added jfrview, esbuild-jest dependencies and Jest configuration for WASM module mocking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
The failure is probably because of the rename I did as part of copilot's review (so my fault, not yours) -- I'll check this out later. Thanks @beatbrot! |
|
@beatbrot This is now live on https://speedscope.app and published to npm as [email protected] Two additional things...
|
Sure! I don't know how to contribute to the wiki, so I attached a version to this comment :)
Hmmm okay... to be honest, I have no idea how we could solve this properly though... |
|
Thanks! Just updated here: https://github.com/jlfwong/speedscope?tab=readme-ov-file#supported-file-formats
I think for now we just ignore it and see if anyone cares. If they do, this is, I believe, fixable by base64 encoding the wasm binary and putting it into a lazy-loaded |
As promised in #397: JFR file support.
I did not add tests since the test setup for WASM files is just too cumbersome. Hope this is okay :)