bytecodealliance / ComponentizeJS

JS -> WebAssembly Component
Apache License 2.0
242 stars 32 forks source link

deps: update to latest starlingmonkey #111

Closed guybedford closed 5 months ago

guybedford commented 6 months ago

Updates to the latest version of StarlingMonkey as of https://github.com/bytecodealliance/StarlingMonkey/commit/d7c63482392bcde2882503f77d4ad75f85f36d50.

karthik2804 commented 6 months ago

Thank you for this PR! This is great to see how to update starlingmonkey. When building this PR and running the tests using npm run build && npm run test I get the following errors on every binding test.

Splicer error: unable to read prelude sequence, continuing for debug build but note binding functions will not work!
guybedford commented 6 months ago

@karthik2804 make sure you are definitely not using a debug build or referencing the debug build in this project, as that error is only associated with debug builds.

karthik2804 commented 6 months ago

I just attempted it with a fresh clone of the project with the following commands and I get the same error

git clone https://github.com/bytecodealliance/ComponentizeJS
cd ComponentizeJS
gh pr checkout 111
npm install
git submodule update --init --recursive
npm run build && npm run test
 Builtins
    1) console-object
    ✔ console-simple (2177ms)
    ✔ crypto-random-disabled (2222ms)
    ✔ crypto-random (2230ms)
    2) globals
    ✔ math-random-disabled (2206ms)
    ✔ math-random (2173ms)
    ✔ now-disabled (2211ms)
    ✔ now (2215ms)
    ✔ pure (2152ms)
    ✔ timeout-disabled (2183ms)
    3) timeout

  Bindings
    ✔ args (2116ms)
Splicer error: unable to read prelude sequence, continuing for debug build but note binding functions will not work!
    4) char
Splicer error: unable to read prelude sequence, continuing for debug build but note binding functions will not work!
    5) conventions
    ✔ empty (2116ms)
Splicer error: unable to read prelude sequence, continuing for debug build but note binding functions will not work!
    6) flags
Splicer error: unable to read prelude sequence, continuing for debug build but note binding functions will not work!
flavorful.bindings.js:147:32 RangeError: invalid or out-of-range index
Redirecting call to abort() to mozalloc_abort

    7) flavorful
Splicer error: unable to read prelude sequence, continuing for debug build but note binding functions will not work!
    8) floats
Splicer error: unable to read prelude sequence, continuing for debug build but note binding functions will not work!
hello.bindings.js:111:32 RangeError: offset is outside the bounds of the DataView
Redirecting call to abort() to mozalloc_abort

    9) hello
Splicer error: unable to read prelude sequence, continuing for debug build but note binding functions will not work!
http-request.js:24:45 TypeError: futureIncomingResponse.get() is undefined
Redirecting call to abort() to mozalloc_abort

    10) http-request
Splicer error: unable to read prelude sequence, continuing for debug build but note binding functions will not work!
    ✔ import-func (3910ms)
guybedford commented 6 months ago

Ahh thanks for spotting that, the splicing functions might need to be updated to the latest binary format here then due to the Spidermonkey upgrade.

karthik2804 commented 6 months ago

I did a little print line debugging on https://github.com/bytecodealliance/ComponentizeJS/blob/519256f5632f48e42cfea54bfd588f383eccb855/crates/spidermonkey-embedding-splicer/src/splice.rs#L231

The snippet below is a part of the output.

InstrSeq {
    ...
    instrs: [
        (
            LocalGet(
                LocalGet {
                    local: Id {
                        idx: 21481,
                    },
                },
            ),
            ...
}
guybedford commented 6 months ago

@karthik2804 I fixed up the splicing portion of this PR so those tests should be passing now, if that helps.

karthik2804 commented 6 months ago

@guybedford Thank you! I can confirm that the tests are passing now. I will now attempt to see if I can get fetch to work when building an app with componentize-js.

karthik2804 commented 6 months ago

@guybedford Looking at the diff with respect to starlingmonkey it no longer includes the fetch rework unless I am missing something. https://github.com/bytecodealliance/StarlingMonkey/compare/c1d7439a3412496db0c5d4004d723396cc46a892...5bdb946f137e946206fb865cdba92b8fb0654416

guybedford commented 6 months ago

@karthik2804 it needs a merge of that work against https://github.com/bytecodealliance/StarlingMonkey/pull/56 now, I've tried to add that, let's see how it looks in CI.

karthik2804 commented 6 months ago

Fetch seems to be supported but it does look like async is broken. The guest appears to be terminating on the first await call. A minimal reproduction can be achieved by modifying the sample in the examples/ folder of this repo.

// hello.js
export function hello(name) {
  console.log("in here")
  test()
  return `Hello ${name}`;
}

async function test() {
  console.log("in test")
  await new Promise(resolve => setTimeout(resolve, 0));
  console.log("after await")
}

The result of running node componentize.js && cargo run --release is

in here
in test
Hello ComponentizeJS

The output when running from main

in here
in test
after await
Hello ComponentizeJS

This is possibly a starlingmonkey issue.

guybedford commented 5 months ago

@karthik2804 I've posted https://github.com/bytecodealliance/ComponentizeJS/issues/112 with an outline of an approach, happy to discuss further if it helps.

guybedford commented 5 months ago

@karthik2804 here's what I do to implement async in the mean time - a run and ready polling implemention is used in the tests here - https://github.com/bytecodealliance/ComponentizeJS/blob/main/test/test.js#L25.

karthik2804 commented 5 months ago

@guybedford Thank you for the example implementation. I am trying to target the incomingHandler export of wasi:http where I do not believe this polling implementation applies since I am not in control of the host side implementation/wit.

guybedford commented 5 months ago

In that case, an implementation along the lines of #112 sounds like the right kind of approach to me.