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

feat: support first class function via builtin function #2752

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HerrCai0907
Copy link
Member

This is the first pr to support first class function.
Firstly we need to support allocating function object in heap instead of data area.

@CountBleck
Copy link
Member

Are you sure we shouldn't wait until we use Wasm GC?

@HerrCai0907
Copy link
Member Author

Are you sure we shouldn't wait until we use Wasm GC?

I think we can do it separately. First class is based on object module, it don't and shouldn't rely on wasm GC or __new. It only means we have a Function class and can manage it as normal object.

src/diagnostics.ts Outdated Show resolved Hide resolved
@CountBleck
Copy link
Member

By "first class function", do you mean closures? Those might be easier to implement in Wasm GC, since the heap doesn't have to be dealt with manually, and closure objects for child blocks would include the parent closure objects (at least the way I see it).

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented Sep 29, 2023

Those might be easier to implement in Wasm GC, since the heap doesn't have to be dealt with manually, and closure objects for child blocks would include the parent closure objects (at least the way I see it).

We have memory allocation abstraction, and then we have GC object abstraction based on memory allocation. Those thing can be replaced by
WASM GC. But closures should be something based on those thing. When we design closures, we should not consider where and how to create object. It should be same as new XXX().

src/compiler.ts Outdated Show resolved Hide resolved
@CountBleck
Copy link
Member

It should be same as new XXX().

That's actually my point. The XXX is backed by a Class, which is backed by a ClassPrototype, which is backed by an AST node (ClassDeclaration). Sharing code with this would be difficult (at least, I think so), and not sharing code would add another arcane part of the compiler that'd be harder to maintain or iterate on (as far as I can see).

Meanwhile, with Wasm GC, we could just make GC structs without touching AS's type system (which would also be simplified). As a result, it would (theoretically) be much more convenient to write, debug, and maintain.

@CountBleck
Copy link
Member

Also, the current test should already work: see here.

@HerrCai0907
Copy link
Member Author

Also, the current test should already work: see here.

Yes. This pr will not support closure. It only wants to support store function instance in heap instead of in data section. I think it will be the first step to support closure.

@CountBleck
Copy link
Member

I don't quite understand what you mean.

@CountBleck
Copy link
Member

Oh, do you mean creating a new Function object for each returned inner function, instead of a single object?

@HerrCai0907 HerrCai0907 mentioned this pull request Sep 30, 2023
6 tasks
@HerrCai0907 HerrCai0907 marked this pull request as draft September 30, 2023 07:17
@HerrCai0907
Copy link
Member Author

This is binaryen bug which fixed in latest binaryen.

# compiler/first-class-function

- compile debug
  SUCCESS (251.300 ms)

- compare fixture
  SUCCESS (1.495 ms)

- compile release
  SUCCESS (397.126 ms)

- compare fixture
  SUCCESS (0.117 ms)

- instantiate debug
  SUCCESS (1.620 ms)

- instantiate release
  ---
  RuntimeError: table index is out of bounds
      at wasm://wasm/651ca9aa:wasm-function[14]:0xc45
  FAILURE (1.120 ms)
      at wasm://wasm/651ca9aa:wasm-function[13]:0xb6d

  ---

@lcswillems
Copy link

@HerrCai0907 Great news you are working on this!! 🙏 @CountBleck Is Wasm GC ready?

@CountBleck
Copy link
Member

Is Wasm GC ready?

Nope. Keep in mind that I'm a procrastinating 10th grader who has to deal with an annoying amount of English homework :P

I'll try to get in more work towards the Wasm GC transition in my spare time, but the progress won't really be visible until I make a PR to Binaryen.

@HerrCai0907 HerrCai0907 force-pushed the feat/first-class-1 branch 5 times, most recently from 5705cc3 to 758e86a Compare November 10, 2023 05:53
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.

None yet

3 participants