Skip to content

Initial implementation of the WebAssembly backend. #4988

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

Merged
merged 4 commits into from
Aug 10, 2024

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented May 23, 2024

This commit contains the initial implementation of the WebAssembly backend. This backend is still experimental, in the sense that:

  • We may remove it in a future Minor version, if we decide that it has a better place elsewhere, and
  • Newer minor versions may produce WebAssembly code that requires more recent WebAssembly features.

The WebAssembly backend silently ignores @JSExport and @JSExportAll annotations. It is otherwise supposed to support the full Scala.js language semantics.

Currently, the backend only supports some configurations of the linker. It requires:

  • No optimizer,
  • Unchecked semantics for undefined behaviors,
  • Strict floats, and
  • ES modules.

Some of those will be relaxed in the future, definitely including the first two.

Co-authored-by: @tanishiking


Past history of the development of this backend may be found at https://github.com/tanishiking/scala-wasm
We did not make any effort in preserving the git history when creating this PR, as it followed a very aggressive commit policy, which would not fit this repository anyway.

@sjrd
Copy link
Member Author

sjrd commented May 24, 2024

The build is now green. The code should be good to go. I have started some Readme documentation of the compilation scheme, to complement the code comments which are more local. I'll continue in the coming days.

@gzm0 I expect reviewing this will take time (like, weeks or more). Beside the compilation scheme, are there things you would like to see and that would help you review?

I'm already marking you as reviewer, but it's fine if you wait for me to provide everything you will need before starting, obviously.


Also general note about future edits: from now on I will refrain from force-pushing on this branch. I will only add commits, even if they are wip addressing comments (but hopefully each passing the CI regardless). We'll rebase when all the reviewing process over.

@sjrd sjrd requested a review from gzm0 May 24, 2024 16:15
@gzm0
Copy link
Contributor

gzm0 commented May 25, 2024

Beside the compilation scheme, are there things you would like to see and that would help you review?

I think for now, this is OK. My plan was to first focus on the "outer structure" anyways, so for this, I do not need to know a lot (anything?) about the compilation scheme.

One thing that I'd like to get out of this PR is to get a better understanding how reviews will work going forward in this repository between @sjrd, @tanishiking and me. Basically, I'd like to understand if we need more "formal" structure to ensure that:

  • Everybody stays in the loop about changes relevant to their area of expertise.
  • We do not waste resources by having multiple people look at changes (I have a feeling this already happens sometimes between @sjrd and me when we have external contributions).

In any case though, I think we'll be better equipped at the end of the review of this PR to answer this question. I just wanted to put it out there, so we do not forget.

from now on I will refrain from force-pushing on this branch

FWIW: For me Github's "viewed" state is likely going to be much more important than individual commits. So for example, feel free to amend the README.md comment as much as you'd like until it is ready for review.

Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

First review pass on everything but wasmemitter and webassembly.

@gzm0
Copy link
Contributor

gzm0 commented May 25, 2024

One thing @sjrd you might be able to provide that helps me, is a "suggested order of review" for the two new packages. (I was assuming webassembly should go first, but maybe you have a different, more informed suggestion).

@sjrd
Copy link
Member Author

sjrd commented May 25, 2024

One thing @sjrd you might be able to provide that helps me, is a "suggested order of review" for the two new packages. (I was assuming webassembly should go first, but maybe you have a different, more informed suggestion).

Good point. I would suggest the following:

In webassembly:

  1. Start with the set Identities, Types, Modules, Instructions. They define our core immutable model of WebAssembly.
  2. ModuleBuilder
  3. FunctionBuilder, but ignore the body of def switch as well as def localDeadCodeEliminationOfInstrs (you can come back to those later after getting more comfortable with straightforward Wasm codegen)

This should be enough of webassembly to understand the rest. BinaryWriter and TextWriter can be reviewed independently of everything else. They can be good "lighter review" to do when you don't have much time to commit.

In wasmemitter:

  1. Start with EmbeddedConstants and SpecialNames (they are trivial but some of their comments give context)
  2. Review TypeTransformer
  3. Read the doc comments of VarGen.genFieldID.typeData and its type members
  4. Read CoreWasmLib.genNewArrayOfThisClass to get familiar with a) our "DSL" to emit Wasm instructions and b) what working with Wasm instructions looks like. I suggest this one because a) it is a direct equivalent of newArrayOfThisClass in CoreJSLib, b) it manipulates virtually no larger concept of our compilation scheme and c) it is non-trivial nevertheless.
  5. Read LoaderContent once (or the generated __loader.js file for a project like the helloworld) to get an idea of what functions exist in there.
  6. Read the headers of the other def genX methods of CoreWasmLib (not their implementation), to also get an idea of what helper functions exist.
  7. At this point you should have enough overall context to get to get to the meat, so:
  8. Read the "API" of WasmContext and WasmContext.ClassInfo.
  9. Review FunctionEmitter. Keep genApplyWithDispatch for the end. Only do the "HERE BE DRAGONS" part if you feel adventurous.
  10. Review SWasmGen
  11. ClassEmitter, but ignore the monster genCreateJSClassFunction
  12. Emitter
  13. CoreWasmLib
  14. WasmContext
  15. Preprocessor
  16. and finally what's left in no particular order

Copy link
Contributor

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

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

I haven't looked at the code for the compilation scheme much, only checking the differences from the book's repository. (Since I've reviewed the most of code there through the interaction in scala-wasm repo).
I left a few comments but it looks good to me, and wasmemitter/README.md looks great so far.

@gzm0
Copy link
Contributor

gzm0 commented May 25, 2024

FYI, I'll mark comments I made as resolved once I feel the discussion is resolved. @tanishiking @sjrd do you feel we need to agree on a scheme on how to resolve comments? (e.g. opener only resolves?)

@sjrd
Copy link
Member Author

sjrd commented May 25, 2024

I think opener resolves, unless it's a typo or something like that.

@sjrd
Copy link
Member Author

sjrd commented May 26, 2024

I believe I have addressed everything so far.

  • The createJSError fix was integrated in its original commit, to keep being a stand-alone.
  • I now squashed the change to deriving LinkedClasses into the big commit, since that actively reduces the diff.
  • The rest is in small separate commits for now.

@sjrd sjrd force-pushed the webassembly branch 2 times, most recently from 6f64fc2 to 5b1cfbd Compare May 27, 2024 12:27
Copy link
Contributor

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

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

I know the README is WIP, but LGTM from my side 💯

@sjrd sjrd force-pushed the webassembly branch 2 times, most recently from 23e3538 to 86b8f6c Compare May 28, 2024 10:01
@sjrd
Copy link
Member Author

sjrd commented May 28, 2024

I think I'm done with the README at this point. There are many more things I could write about, but they are more local, so comments in the code should cover them well. I focused on things that have a global impact across the backend.

Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Addressed comments in ClassEmitter and added details about the overall ordering of definitions in the Readme.

Sorry for the delay in addressing the latest batch. I'm mostly on vacation these days.

Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Addressed comments in CoreWasmLib, DerivedClasses, and some comments in Emitter but definitely not all of them yet.

Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Review of most recent changes and answers to some comments.

I have also done a full "resolution" pass.

Sorry for the delay in addressing the latest batch. I'm mostly on vacation these days.

There is no need to apologize. It's not like I don't have anything to do anymore :P

Enjoy your vacation (if you are still off :)).

Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Addressing some more comments.

Main todos:

  • Moving from Emitter to ClassEmitter stuff in the start function (not yet convinced that would be better)
  • Extracting StringPool
  • Comments in TextWriter

Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Phew, I think I have addressed everything so far.

Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Oops, I had forgotten a bunch of comments in TextWriter.

sjrd added 3 commits July 28, 2024 14:43
If an object is sealed, `captureStackTrace` throws an exception.
This will happen for WebAssembly objects.

We now detect this case and fall back to instantiating a dedicated
`js.Error` object.
We now directly use `import("./main.js")` or `require("./main.js")`
rather than relying on the compilation scheme of `js.dynamicImport`.

This will allow `ExportLoopback` to work under WebAssembly,
although the initial implementation will not support multiple
modules.
Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Review of everything except BinaryWriter and the points I have explicitly left comments (IIRC, primarily the dragons).

Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Addressed some comments. I need to look more deeply into the possible bug in computeItableBuckets.

@sjrd
Copy link
Member Author

sjrd commented Jul 30, 2024

Well ... it turns out we didn't need Phase 2 of the bucket assignment algorithm at all! I added a few commits that should address the latest comments on that algorithm.

@tanishiking Could you double-check what I did there?

Here is a gist with the bucket assignments computed for the helloworld and for the test suite:
https://gist.github.com/sjrd/0c10d4cdf43e5a8796ed6d0619edfb34
It's interesting to note that there are barely more buckets for the full test suite than for the helloworld. We basically get 40 buckets because of the huge mess that is the collections hierarchy, but not much more beyond that.

(also the recent changes did not alter the number of buckets allocated for the test suite -- I did not check the helloworld)

Copy link
Contributor

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

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

Thanks, I left one question, and otherwise the change looks reasonable 👍

It's interesting to note that there are barely more buckets for the full test suite than for the helloworld. We basically get 40 buckets because of the huge mess that is the collections hierarchy, but not much more beyond that.

Yeah, that happens when a class has a bunch of superclasses/interfaces, as all those superclasses share the same subclass and therefore can't be assigned to the same bucket. This situation applies to the hierarchy in the Scala collection libraries :/

I believe most applications have that number of buckets, but not many go beyond that, as long as the applications define a more deeply nested data hierarchy than the Scala collections.

shapes at 24-07-31 13 34 54

Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Review of BinaryWriter (mostly minor things) & follow-up changes (IIRC no additional comments).

At this point, all remaining things to be done are in unresolved conversations (some of them are TODOs for myself).

Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Addressed comments in BinaryWriter, but not elsewhere yet.

@sjrd
Copy link
Member Author

sjrd commented Aug 4, 2024

And I addressed the remaining comments that were not resolved yet, AFAICT.

Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Complete review. The dragons seem to be implemented nicely. So it is not that terrible to understand :)

Copy link
Member Author

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Addressed latest batch of comments.

@sjrd sjrd requested a review from gzm0 August 10, 2024 13:01
Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

LGTM after squashing (down to 4 total commits).

This commit contains the initial implementation of the WebAssembly
backend. This backend is still experimental, in the sense that:

* We may remove it in a future Minor version, if we decide that it
  has a better place elsewhere, and
* Newer minor versions may produce WebAssembly code that requires
  more recent WebAssembly features.

The WebAssembly backend silently ignores `@JSExport` and
`@JSExportAll` annotations. It is otherwise supposed to support
the full Scala.js language semantics.

Currently, the backend only supports some configurations of the
linker. It requires:

* No optimizer,
* Unchecked semantics for undefined behaviors,
* Strict floats, and
* ES modules.

Some of those will be relaxed in the future, definitely including
the first two.

Co-authored-by: Rikito Taniguchi <rikiriki1238@gmail.com>
@sjrd
Copy link
Member Author

sjrd commented Aug 10, 2024

Thank you so much @gzm0 for this epic review!

@sjrd sjrd merged commit 76e3292 into scala-js:main Aug 10, 2024
3 checks passed
@sjrd sjrd deleted the webassembly branch August 10, 2024 17:01
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.

4 participants