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

deps: update to latest starlingmonkey #111

Merged
merged 13 commits into from
Jun 28, 2024
Merged

Conversation

guybedford
Copy link
Collaborator

@guybedford guybedford commented May 28, 2024

Updates to the latest version of StarlingMonkey as of bytecodealliance/StarlingMonkey@d7c6348.

@karthik2804
Copy link
Contributor

karthik2804 commented May 28, 2024

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
Copy link
Collaborator Author

@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
Copy link
Contributor

karthik2804 commented May 28, 2024

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
Copy link
Collaborator Author

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
Copy link
Contributor

I did a little print line debugging on

let prelude_block = &coreabi_sample_i32

The snippet below is a part of the output.

InstrSeq {
    ...
    instrs: [
        (
            LocalGet(
                LocalGet {
                    local: Id {
                        idx: 21481,
                    },
                },
            ),
            ...
}

@guybedford
Copy link
Collaborator Author

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

@karthik2804
Copy link
Contributor

@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
Copy link
Contributor

@guybedford Looking at the diff with respect to starlingmonkey it no longer includes the fetch rework unless I am missing something.
bytecodealliance/StarlingMonkey@c1d7439...5bdb946

@guybedford
Copy link
Collaborator Author

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

@karthik2804
Copy link
Contributor

karthik2804 commented May 30, 2024

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
Copy link
Collaborator Author

@karthik2804 I've posted #112 with an outline of an approach, happy to discuss further if it helps.

@guybedford
Copy link
Collaborator Author

@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 karthik2804 mentioned this pull request May 30, 2024
@karthik2804
Copy link
Contributor

@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
Copy link
Collaborator Author

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

@guybedford guybedford mentioned this pull request Jun 28, 2024
@guybedford guybedford merged commit ca43536 into main Jun 28, 2024
2 checks passed
@guybedford guybedford deleted the wip-latest-starlingmonkey branch June 28, 2024 15:25
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.

2 participants