Add Blob builtin implementation #169
Conversation
There was a problem hiding this comment.
To update the WPT coverage, you need to create a JSON file in https://github.com/bytecodealliance/StarlingMonkey/tree/main/tests/wpt-harness/expectations for the corresponding tests you add, corresponding to the same test name but as .any.js.json instead of .any.js.
These files can be auto-generated with the --update-expectations WPT flag in https://github.com/bytecodealliance/StarlingMonkey/blob/main/tests/wpt-harness/run-wpt.mjs#L79.
WPT flags can be set when doing a manual WPT run via WPT_FLAGS="--update-expectations" which is covered at the end of https://github.com/bytecodealliance/StarlingMonkey?tab=readme-ov-file#web-platform-tests.
If the file name defined in tests.json cannot be found, the fetch
function doesn’t throw an error. Instead, it writes the error into the
response. Without checking the response status, we proceed to evaluate
the script source, which in this case is:
`Error: {"error": {"code": 404, "message": "404"}}`
At this point, eval throws an Invalid Syntax error without indicating
that the file wasn't fetched successfully.
|
Is |
It absolutely is, yes. We have some gaps on byte streams specifically, but You probably need to make use of NativeStreamSource, which we're currently using for |
Hey @tschneidereit I looked at the body stream implementation, but I'm not sure how to adapt it for a The part I'm uncertain about is that the |
|
@andreiltd you can follow the same process as |
|
Thanks @guybedford . I considered using a task because this is what spec mentions as well: https://w3c.github.io/FileAPI/#blob-get-stream but I probably just misunderstood what needs to be done here :) Thanks for clarification! |
|
Ahh I see, right we have no concept of immediate tasks to add to the task list which do not depend on WASI polls currently. One option might be to use Alternatively we do have the concept of tasks having a Either sounds to me like it would get us semantic correctness in draining the microtasks first before draining tasks, and Fastly's Perhaps @tschneidereit may have more thoughts on this as well. |
|
The suggested code seems to work. Thanks! But it looks like there is something wrong with the streaming for large blobs (>10M). I've added the test that creates a blob and read it in chunks. There is also an interval task running concurrently. This all seems to be working except if I increase the size of the blob to 10M or 100M I get the size mismatch between the amount of bytes read from the stream and actual blob size. The stream size is always larger. I will have to debug this :) |
I think I’ve got to the root of the problem: I’m storing references to GC-managed objects in the blob slot. This slot is a map that allows lookup of the reader state using the stream pointer.: This long running test triggers a GC run which invalidates the stored pointers and as a result it restarts the reading of the stream. I guess if I want to fix that I need to change the references to EDIT: I would imagine something like my last commit should allow tracing: 0c349f0 |
|
@andreiltd have all the review comments here been addressed now? |
I think so, yes. |
|
Thank you @tschneidereit and @guybedford for all the in-depth feedback! I'm confident that the scars from my first PR have taught me enough to be much more productive with future ones :) |
|
Congrats on a fantastic first PR here 🎉 |
This patch adds support for the Blob API.
The current status of platform tests is
205216 passes out of 222 tests. Notable failures include: