Avoid unnecessary allocations in HTMLRewriter #2061
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
requested review from
Frederik-Baetens,
jp4a50,
dom96 and
harrishancock
The code was allocating an IdentityTransformStream and WritableStream just to end up throwing both away immediately after.
jasnell
force-pushed
the
jsnell/htmlrewriter-avoid-unnecessary-allocation
branch
from
0459554
to
71dcf5b
Compare
jasnell
commented
Apr 26, 2024
| auto ts = IdentityTransformStream::constructor(js); | ||
| response = Response::constructor(js, kj::Maybe(ts->getReadable()), kj::mv(response)); | ||
|
|
||
| auto outputSink = ts->getWritable()->removeSink(js); |
There was a problem hiding this comment.
For reviewers: If you don't have the context on this change, what is happening in the original code here is that we allocate an IdentityTransformStream, which creates an internal refcounted IdentityTransformStreamImpl instance, then uses that to create one WritableStream instance and one ReadableStream instance. We then immediately extract the reference to the IdentityTransformStreamImpl out of the WritableStream. In this case, creating both the IdentityTransformStream and the WritableStream are unnecessary since both are immediately thrown away.
In the new code, we create only the IdentityTransformStreamImpl and the ReadableStream that we are keeping.
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
garrettgu10
pushed a commit
that referenced
this pull request
May 13, 2024
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.
The original code was allocating an
IdentityTransformStreamandWritableStreamjust to end up throwing both away immediately after. This simplifies things by making it possible to create a the identity pipeline that underliesIdentityTransformStreamdirectly, simplifying things a bit by avoiding the unnecessary additional allocations and removing an unnecessary use of theremoveSink()API.Internal CI is green.