Skip to content

Conversation

@beatbrot
Copy link
Contributor

@beatbrot beatbrot commented Nov 30, 2025

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 :)

@beatbrot beatbrot marked this pull request as ready for review November 30, 2025 14:08
Copy link
Owner

@jlfwong jlfwong left a 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...
@beatbrot
Copy link
Contributor Author

beatbrot commented Dec 1, 2025

@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.

@jlfwong
Copy link
Owner

jlfwong commented Dec 1, 2025

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

@beatbrot
Copy link
Contributor Author

beatbrot commented Dec 1, 2025

@jlfwong Sorry for the lacking explanation on my side :) I'll explain what I had to do:

I think I'm not fully following what babel is needed for here

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 import.meta.url which is not avaible in NodeJS environments. Therefore I also needed babel-plugin-transform-import-meta.

For example, we have pre-existing tests which depend on wasm component being loaded in jest

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 init function that accepts either an URL to the wasm file or an ArrayBuffer containing the wasm bytecode. While this is a bit more complicated, it gives us proper lazy loading.

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.

@jlfwong
Copy link
Owner

jlfwong commented Dec 1, 2025

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

@coveralls
Copy link

coveralls commented Dec 2, 2025

Coverage Status

coverage: 43.712% (+0.2%) from 43.466%
when pulling 5669fb4 on beatbrot:java-flight-recorder-support
into 40b2fea on jlfwong:main.

@beatbrot
Copy link
Contributor Author

beatbrot commented Dec 2, 2025

@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?

@jlfwong jlfwong requested a review from Copilot December 2, 2025 18:40
Copilot finished reviewing on behalf of jlfwong December 2, 2025 18:46
Copy link
Contributor

Copilot AI left a 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.

@jlfwong
Copy link
Owner

jlfwong commented Dec 2, 2025

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!

@jlfwong jlfwong merged commit 51a1639 into jlfwong:main Dec 3, 2025
7 checks passed
@jlfwong
Copy link
Owner

jlfwong commented Dec 3, 2025

@beatbrot This is now live on https://speedscope.app and published to npm as [email protected]

Two additional things...

  1. Can you please write a markdown file like the one here explaining how to use this? https://github.com/jlfwong/speedscope/wiki/Importing-from-async%E2%80%90profiler-(Java)
  2. The downside of this using fetch to grab the wasm data is the JFR import won't work when speedscope is used as static files w/ no server locally. This is a workflow we've historically supported, as described here: https://github.com/jlfwong/speedscope?tab=readme-ov-file#self-contained-directory. It seems like browsers are hellbent on kneecapping support for features that aren't running under https (including not running under file:// protocol), so perhaps this is fine. If someone hits this, they can report it and we can figure out how to address

@beatbrot
Copy link
Contributor Author

beatbrot commented Dec 5, 2025

@jlfwong

Can you please write a markdown file like the one here explaining how to use this?

Sure! I don't know how to contribute to the wiki, so I attached a version to this comment :)
Importing-from-JFR-.Java.md

The downside of this using fetch to grab the wasm data is the JFR import won't work when speedscope is used as static files w/ no server locally.

Hmmm okay... to be honest, I have no idea how we could solve this properly though...

@jlfwong
Copy link
Owner

jlfwong commented Dec 5, 2025

Thanks! Just updated here: https://github.com/jlfwong/speedscope?tab=readme-ov-file#supported-file-formats

Hmmm okay... to be honest, I have no idea how we could solve this properly though...

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 .js script so it doesn't need to be fetched from a file:// protocol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants