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

fix: only apply our pipeTo/pipeThrough optimisations to TransformStreams who have no transformers (IdentityStreams). #556

Merged
merged 2 commits into from Jun 9, 2023

Conversation

JakeChampion
Copy link
Contributor

@JakeChampion JakeChampion commented Jun 9, 2023

We have a performance optimisation for when the receiver is a native source, whereby we append the entire source body to the destination using a single native hostcall, this greatly improves the performance (https://barstool.engineering/a-real-world-comparison-between-cloudflare-workers-and-fastly-compute-edge/)

This optimisation however, should only be applied when the TransformStream is being used in this particular, having not supplied a transformer to the TransformStream constuctor, i.e. an IdentityStream. If we apply the optimisation when a transformer is supplied, the transformer would never get called, which is not correct behavior.

We want to apply the optimisation in this scenario:

function combineStreams(streams: ReadableStream[]): ReadableStream {
  const stream = new TransformStream()
  _combineStreams(streams, stream.writable)
  return stream.readable
}

async function _combineStreams(sources: ReadableStream[], destination: WritableStream) {
  for (const stream of sources) {
    await stream.pipeTo(destination, {
      preventClose: true
    })
  }
  destination.close()
}

And we don't want to apply the optimisation in any other scenario such as:

function upperCaseStream() {
  return new TransformStream({
    transform(chunk, controller) {
      controller.enqueue(chunk.toUpperCase());
    },
  });
}

@JakeChampion JakeChampion force-pushed the jake/transform-stream branch 2 times, most recently from e63c8ca to 5a69ed6 Compare June 9, 2023 10:38
@JakeChampion JakeChampion changed the title fix: only apply our pipe_to/pipe_through optimisations to TransformStreams who have no transformers. fix: only apply our pipeTo/pipeThrough optimisations to TransformStreams who have no transformers. Jun 9, 2023
@JakeChampion JakeChampion changed the title fix: only apply our pipeTo/pipeThrough optimisations to TransformStreams who have no transformers. fix: only apply our pipeTo/pipeThrough optimisations to TransformStreams who have no transformers (IdentityStreams). Jun 9, 2023
…reams who have no transformers.

We have a performance optimisation for when the receiver is a native source, whereby we append the entire source body to the destination using a single native hostcall, this greatly improves the performance (https://barstool.engineering/a-real-world-comparison-between-cloudflare-workers-and-fastly-compute-edge/)

This optimisation however, should only be applied when the TransformStream is being used in this particular, having not supplied a transformer to the TransformStream constuctor. If we apply the optimisation when a transformer is supplied, the transformer would never get called, which is not correct behavior.
@JakeChampion JakeChampion marked this pull request as ready for review June 9, 2023 10:55
Copy link
Contributor

@triblondon triblondon left a comment

Choose a reason for hiding this comment

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

👍 for the test case

Copy link
Contributor

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Looks great!

@JakeChampion JakeChampion merged commit a88616c into main Jun 9, 2023
79 of 80 checks passed
@JakeChampion JakeChampion deleted the jake/transform-stream branch June 9, 2023 16:53
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.

None yet

3 participants