src: implement DataQueue and non-memory resident Blob #45258
Closed
+2,750
−456
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcollina
reviewed
Nov 1, 2022
|
@mcollina ... this is ready for review! |
mcollina
reviewed
Dec 18, 2022
|
cc @KhafraDev |
This comment was marked as outdated.
This comment was marked as outdated.
14 tasks
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
9b037f1
to
18ba98e
Compare
14 tasks
This comment was marked as outdated.
This comment was marked as outdated.
12 tasks
addaleax
previously requested changes
Dec 21, 2022
14 tasks
|
Moving this back to Draft status. Trying to work through @addaleax's feedback and wanted to make it so that the |
13 tasks
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@jasnell It seems the test failure for the ASAN build is relevant: https://github.com/nodejs/node/actions/runs/4190697226/jobs/7264365006#step:6:5307 |
See documentation in dataqueue/queue.h for details Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
24 tasks
jasnell
added a commit
that referenced
this pull request
Feb 20, 2023
See documentation in dataqueue/queue.h for details Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com> PR-URL: #45258 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell
added a commit
that referenced
this pull request
Feb 20, 2023
Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com> PR-URL: #45258 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell
added a commit
that referenced
this pull request
Feb 20, 2023
Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com> PR-URL: #45258 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
Landed in 9a82938...71fb06f |
26 tasks
|
Would you mind to backporting this to v18.x? |
targos
pushed a commit
that referenced
this pull request
Mar 13, 2023
See documentation in dataqueue/queue.h for details Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com> PR-URL: #45258 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos
pushed a commit
that referenced
this pull request
Mar 13, 2023
Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com> PR-URL: #45258 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos
pushed a commit
that referenced
this pull request
Mar 13, 2023
Co-authored-by: flakey5 <73616808+flakey5@users.noreply.github.com> PR-URL: #45258 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos
added a commit
that referenced
this pull request
Mar 14, 2023
Notable changes: buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 src: * (SEMVER-MINOR) add `fs.openAsBlob` to support File-backed Blobs (James M Snell) #45258 tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 wasi: * (SEMVER-MINOR) add support for version when creating WASI (Michael Dawson) #46469 worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47086
targos
added a commit
that referenced
this pull request
Mar 14, 2023
Notable changes: buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 src: * (SEMVER-MINOR) add `fs.openAsBlob` to support File-backed Blobs (James M Snell) #45258 tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 wasi: * (SEMVER-MINOR) add support for version when creating WASI (Michael Dawson) #46469 worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47087
targos
added a commit
that referenced
this pull request
Mar 14, 2023
Notable changes: buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 src: * (SEMVER-MINOR) add `fs.openAsBlob` to support File-backed Blobs (James M Snell) #45258 tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 wasi: * (SEMVER-MINOR) add support for version when creating WASI (Michael Dawson) #46469 worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47087
targos
added a commit
that referenced
this pull request
Mar 14, 2023
Notable changes: buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 src: * (SEMVER-MINOR) add `fs.openAsBlob` to support File-backed Blobs (James M Snell) #45258 tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 wasi: * (SEMVER-MINOR) add support for version when creating WASI (Michael Dawson) #46469 worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47087
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
This is
the start ofworkbeingcollaboratively done by myself and @flakey5 to update the implementation ofBlobto support non-memory resident data sources such as files as well as enable proper streaming support.It is a work in progress and remain draft status until it is ready to go. Opening it for transparency.Some background:
Currently,
Blobobjects consist solely of memory-resident data, backed by a collection ofv8::BackingStoreinstances. While this behavior is correct standard/spec defined behavior, it is extremely inefficient. To support other use cases ofBlob, we want to be able to efficiently and effectively support non-memory resident data sources such as files. To do so will require a number of key changes on the internals of theBlobclass.Further, the current implementation of
Blobsuffers from an overly simplistic streaming model where the entireBlobis first read into a singleArrayBufferbefore passing the data on to the stream API. Effectively meaningBlobsare never actually streamed.While going through this evaluation, we came to realization that many of the underlying requirements for
Blobare (surprisingly) shared by the internal data management requirements for the QUIC implementation that is in progress, and can enable a range of other use cases also, so we have decided to take an approach that addresses those multiple use cases.When complete, this PR is going to do two things:This PR does three things:
DataQueuewhich acts as a sequence of memory-resident and non-memory-resident data sources that can be consumed as a single logical stream of data. This is designed to meet the needs of bothBloband QUIC initially (the bits relevant to QUIC will come later)Blobimplementation to useDataQueueinternally in support of the existing all-memory-resident model.Blobthat will support efficient, non-memory resident use cases.This PR is far from complete. I would strongly recommend that you hold off performing any review on it at all until after we flip the bit to mark it ready for review. We are opening this draft PR now purely for the sake of transparency so that folks can be aware of what we are working on.I also want to avoid any and all bikeshedding on the design until we at least have the updatedBlobimplementation ready to go, and many of the design decisions made will likely not be clear until that concrete case is implemented.Specifically for Blob, this PR
will enableenables:Blobdata be collected into a singleArrayBufferand then passed on to the stream, but data will flow into the stream as one would actually expect.Blob. The ability to acquire aBlobthat is backed by an on-disc file. As well as the ability to use suchBlobinstances efficiently within otherBlobinstances, e.g.new Blob(['string', fileBackedBlob])For QUIC, eventually the
DataQueueimplemented here will replace theStream::Queuemechanism implemented in that PR.DataQueuewill not completely replace that mechanism but covers a sizable chunk of it, which should further simplify and reduce the size of that rather complex bit of work.As for other use cases of
DataQueue... we'll address those later, but we do have a range of cases in mind (including the recent discussions around Node.js providing an efficient http static file server)./cc @mcollina (who prompted this work due to Undici and Fetch use case requirements)