Convert DigestStream into JS-backed stream #2050
Merged
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
jasnell
force-pushed
the
jsnell/refactor-digeststream
branch
from
5819b0c
to
618ba24
Compare
jasnell
added a commit
that referenced
this pull request
Apr 26, 2024
removeSink is used to extract the WritableStreamSink that is owned by a WritableStream using the original implementation. It is currently used in only two places in the codebase, neither of which are necessary. There are two pending changes refactoring those two places to avoid using `removeSink()` as currently defined. Replacing the version of `removeSink` that returns the `WritableStreamSink` with a `detach()` method that returns nothing covers the sockets use case where we call the method then immediately throw away the sink. Once this and the two other PRs land, we can safely remove removeSink entirely. Refs: #2061 Refs: #2050
jasnell
added a commit
that referenced
this pull request
Apr 26, 2024
removeSink is used to extract the WritableStreamSink that is owned by a WritableStream using the original implementation. It is currently used in only two places in the codebase, neither of which are necessary. There are two pending changes refactoring those two places to avoid using `removeSink()` as currently defined. Replacing the version of `removeSink` that returns the `WritableStreamSink` with a `detach()` method that returns nothing covers the sockets use case where we call the method then immediately throw away the sink. Once this and the two other PRs land, we can safely remove removeSink entirely. Refs: #2061 Refs: #2050
jasnell
added a commit
that referenced
this pull request
May 1, 2024
removeSink is used to extract the WritableStreamSink that is owned by a WritableStream using the original implementation. It is currently used in only a few places in the codebase, none of which are necessary. There are two pending changes refactoring those two places to avoid using `removeSink()` as currently defined. Replacing the version of `removeSink` that returns the `WritableStreamSink` with a `detach()` method that returns nothing covers the sockets use case where we call the method then immediately throw away the sink. Once this and the other PRs land, we can safely remove removeSink entirely. Refs: #2061 Refs: #2050 Refs: #2066
dom96
approved these changes
May 8, 2024
|
autogate would be a lot more complicated. I'm the oncall next week so I can easily use one of the daily releases to roll this out in isolation. |
jasnell
force-pushed
the
jsnell/refactor-digeststream
branch
2 times, most recently
from
d53bea1
to
49cf25e
Compare
|
This is ready to go, I'm just going to hold off merging until next week in order to have a dedicated internal release for it. |
The original implementation of `DigestStream` used a WritableStreamSink and the old "internal" streams implementation. This is unnecessary and makes the implementation less efficient that it otherwise could be due to the operations having to bounce between the kj event loop and the JS isolate lock. This change converts `DigestStream` into a JS-backed stream where the operations occur within the isolate lock, avoiding the bounce and avoiding having to reacquire the isolate lock after each chunk, etc. Also avoids the need for IoContext addObject / IoOwn dereferencing. Implementation should be safer and more efficient. Also allow DigestStream to accept string inputs. If a string is written to a DigestStream, it will be converted to UTF-8 bytes and included in the digest. Also allow DigestStream to support `Symbol.dispose`. Improves tests a bit also.
jasnell
force-pushed
the
jsnell/refactor-digeststream
branch
from
49cf25e
to
97cd544
Compare
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.
Suggestion cannot be applied right now. Please check back later.
This is one of a series of PRs intended to simplify and improve the implementation/safety/performance/design of various streams bits in the runtime. This one starts with refactoring
DigestStream.The original implementation of
DigestStreamused aWritableStreamSinkand the old "internal" streams implementation. This is unnecessary and makes the implementation less efficient than it otherwise could be due to the operations having to bounce between the kj event loop and the JS isolate lock. There's just nothing in the implementation ofDigestStreamthat requires the bound through the kj event loop and the original implementation was more an artifact of what was possible at the time it was written.This PR converts
DigestStreaminto a JS-backed stream where the operations occur within the isolate lock. Also avoids the need for IoContext addObject / IoOwn dereferencing. Implementation should be generally safer and more efficient.Also...
DigestStreamto accept string inputs. If a string is written to aDigestStream, it will be converted to UTF-8 bytes and included in the digest.DigestStreamto supportSymbol.dispose.bytesWrittenproperty toDigestStreamto allow for checking to make sure the digest received the expected number of bytes.Internal CI is green.