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

Add Request and Response blob methods #177

Merged

Conversation

andreiltd
Copy link
Contributor

@andreiltd andreiltd commented Nov 25, 2024

This patch adds blob API to Request and Response methods.

The patch also refactors the internal blob storage, changing it from std::vector to mozilla::Vector. This optimization allows the Blob to be created without copying data by transferring buffer ownership, as is the case for both Response and Request objects.

Closes: #121

@andreiltd andreiltd marked this pull request as draft November 25, 2024 12:04
Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

So many more passing tests! ❤️

builtins/web/fetch/request-response.cpp Outdated Show resolved Hide resolved
@andreiltd andreiltd force-pushed the request-response-blob-api branch from d140a41 to 43f2f91 Compare December 3, 2024 12:35
@andreiltd andreiltd mentioned this pull request Dec 4, 2024
This patch allows creating buffer from owned memory by simply
transfering the ownership rather than copying memory.
@andreiltd andreiltd force-pushed the request-response-blob-api branch from 7946178 to 2f697a3 Compare December 5, 2024 21:18
@andreiltd andreiltd marked this pull request as ready for review December 5, 2024 21:25
@guybedford
Copy link
Contributor

guybedford commented Dec 5, 2024

It would also be nice to see in this PR for return new Response(blob) setting the content-type header from the blob type (see the Blob path in https://fetch.spec.whatwg.org/#concept-bodyinit-extract).

Update: I was able to implement these for Fastly here - fastly/js-compute-runtime@9b59afa

@guybedford
Copy link
Contributor

I've also posted #182 as a further follow-up.

@andreiltd
Copy link
Contributor Author

andreiltd commented Dec 6, 2024

Update: I was able to implement these for Fastly here - fastly/js-compute-runtime@9b59afa

Nice! This looks to me like an equivalent of doing:

RootedValue blob_stream(cx);
Call(cx, body_obj, "stream", HandleValueArray::empty(), &blob_stream);

Is there a reason we don't want to use Call? Is this indirection not performant enough? If that's the case I should probably add API that allows calling blob methods like stream, bytes etc. directly. WDYT?

@andreiltd
Copy link
Contributor Author

I've also added support for Blob in structured-clone but perhaps this should be in another PR. Let me know if you want me to split this.

@guybedford
Copy link
Contributor

@andreiltd yeah in general I'd avoid running through the high-level call for performance - we very much need to always keep performance top of mind in development on StarlingMonkey, because by default we are running at a performance penalty compared to other JS platforms, performance is of critical performance for this codebase.

@andreiltd
Copy link
Contributor Author

That makes sense. I will make sure to add interface for calling Blob functions directly in the File API PR.

@guybedford guybedford merged commit c61b935 into bytecodealliance:main Dec 6, 2024
5 checks passed
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.

Blob and Request.blob
3 participants