Skip to content

Conversation

camillobruni
Copy link
Contributor

Runs a in-memory typescript compilation of the immer library.

  • npm run build downloads and build src/gen in-memory file contents for 3 different libs (for local testing)
  • webpack is used to bundle the results
  • There are some type warnings, but all files seem to be loaded

Copy link

netlify bot commented Aug 8, 2025

Deploy Preview for webkit-jetstream-preview ready!

Name Link
🔨 Latest commit f07f378
🔍 Latest deploy log https://app.netlify.com/projects/webkit-jetstream-preview/deploys/68add5924bd99c000851b1e4
😎 Deploy Preview https://deploy-preview-117--webkit-jetstream-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good workload, although sadly on it's last legs... I guess it will become a wasm benchmark when the Go transition happens 🙂

Can we add a README.md with the steps to rebuild?

@camillobruni
Copy link
Contributor Author

Added README and license.

@kmiller68
Copy link
Contributor

Looks like this test also has a bunch of comments that have non-latin1 characters in them. We should probably fix that.

That said, I'm a bit conflicted. It seems like this benchmark combines .ts source files into the final script, which seems odd. I would've expected that the .ts files we're going to parse/compile would come from side files that are loaded into JS strings and subsequently get passed to the typescript compiler. If those source files were non-latin1, I'd be fine with that but since they're mixed with the typescript compiler itself (IIUC, anyway) that seems harder.

@camillobruni
Copy link
Contributor Author

Fair point, let me change that to something where we load the resources (.ts sources) separately and feed them as JS strings to TypeScriptCompileTest.compileTest();. That's also more in line with how other workloads currently handle resources, and it makes it easier to swap out the source projects for local testing.

@camillobruni
Copy link
Contributor Author

  • Updated the benchmark to load the config fils (tsconfig.json and files.json) separate and feed them to the TS compiler
  • Updated webpack to use the UnicodeEscapePlugin just to be sure (will double check again after landing the 2-byte test)

@Constellation
Copy link
Member

@camillobruni @kmiller68 One question I would like to discuss is whether we should include TypeScript test given that TypeScript mainline compiler is moving to golang. https://github.com/microsoft/typescript-go

@camillobruni
Copy link
Contributor Author

That's a fair point.

Some points:

  • This is supposed to replace the existing typescript workloads
  • In general I'm inclined to say that we most urgently need larger JS workloads.
  • In terms of realisticness I feel this one is higher up than most of the old JS workloads
  • I'm a bit worried that we don't enough good JS workloads (good as in large)

In short, if we don't want this one I think we need a solid replacement. I'd weight the need for more diverse workloads currently higher than whether or not it's realistic long term (and this is supposed to be a replacement for the existing typescript).

Alternatives:
Maybe we can turn this one too in a slightly more parse-heavy startup workload? That would balance maybe the workload a bit more?

What's your take on these points?

@kmiller68
Copy link
Contributor

I spoke to Yusuke offline, we think the workload is fine. Maybe we can make the migration to Go slower than the JS version 🙂 and if/when that happens we can replace this with something else for JS4+

@camillobruni
Copy link
Contributor Author

Definitely happy with keeping things a moving target 👍 nothing worse than a non-evovling benchmark suite.

Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, LGTM but we can also address those after merging.

@@ -1824,7 +1824,7 @@ let BENCHMARKS = [
iterations: 15,
worstCaseCount: 2,
deterministicRandom: true,
tags: ["Default", "Octane"],
tags: ["Default", "Octane", "typescript"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should disable this if we're going to enable a more modern version of the same workload.

Comment on lines +2058 to +2059
iterations: 3,
worstCaseCount: 2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this setup worst is the same as average since first is excluded from those measurements. I wouldn't be opposed to running this as a single iteration given how slow it is and that's likely the use case anyway (unless we want to have the latter iterations be incremental via a .tsbuildinfo).

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