fetch: limit web streams usage in body mixin methods

[KhafraDev]

The fetch spec requires us to create a new web Readable stream for every Response and Request object, which can likely be avoided in most cases.

Given the following examples:

new Response('abc')
new Response(new TextEncoder().encode('abc'))
new Response(new Blob(['abc']))
// etc.

A web stream is created and then converted back to a string (or blob, arraybuffer, etc.)! The following diff improves .text() performance when the body passed is a string by 10%, as a simple demo (we could further limit web stream usage for more performance gains!!!).

diff --git a/lib/fetch/response.js b/lib/fetch/response.js
index 1029dbef..c2d2bd3d 100644
--- a/lib/fetch/response.js
+++ b/lib/fetch/response.js
@@ -154,7 +154,7 @@ class Response {
     // 4. If body is non-null, then set bodyWithType to the result of extracting body.
     if (body != null) {
       const [extractedBody, type] = extractBody(body)
-      bodyWithType = { body: extractedBody, type }
+      bodyWithType = { body: extractedBody, type, source: body }
     }

     // 5. Perform initialize a response given this, init, and bodyWithType.
diff --git a/lib/fetch/util.js b/lib/fetch/util.js
index 400687ba..9e4302a9 100644
--- a/lib/fetch/util.js
+++ b/lib/fetch/util.js
@@ -824,6 +824,11 @@ function fullyReadBody (body, processBody, processBodyError) {
   //    with taskDestination.
   const errorSteps = (error) => queueMicrotask(() => processBodyError(error))

+  if (typeof body.source === 'string') {
+    successSteps(new TextEncoder().encode(body.source))
+    return
+  }
+
   // 4. Let reader be the result of getting a reader for body’s stream.
   //    If that threw an exception, then run errorSteps with that
   //    exception and return.

cc @anonrig

[KhafraDev]

benchmarks:

import { Response as UndiciResponse } from './index.js'
import { cronometro } from 'cronometro'

await cronometro({
  async 'undici Response' () {
    await new UndiciResponse('abc').text()
  },
  async 'global Response' () {
    await new Response('abc').text()
  }
})

╔═════════════════╤═════════╤═════════════════╤═══════════╗
║ Slower tests    │ Samples │          Result │ Tolerance ║
╟─────────────────┼─────────┼─────────────────┼───────────╢
║ global Response │   10000 │ 40169.19 op/sec │  ± 1.59 % ║
╟─────────────────┼─────────┼─────────────────┼───────────╢
║ Fastest test    │ Samples │          Result │ Tolerance ║
╟─────────────────┼─────────┼─────────────────┼───────────╢
║ undici Response │   10000 │ 44024.31 op/sec │  ± 2.12 % ║
╚═════════════════╧═════════╧═════════════════╧═══════════╝

[ronag]

I'm -1 for anything that goes against the spec without significant improvements.

[KhafraDev]

I understand, but 10% performance improvement is significant for the small change I made. We still extract the body (convert it to a readable web stream), but instead of reading the bytes from that stream, we use the body's source. The body is still parsed in the same way, the only thing missing is to set the body to used when consumed, which can easily be done. I also don't think the code becomes harder to read or understand, given a comment that explains its purpose.

[anonrig]

I'm -1 for anything that goes against the spec without significant improvements.

I believe this is not against the spec. It is just a shortcut that spec does not care about (at the moment).

I think this is a clever way of improving performance without impacting the rest of the codebase. I’m definitely +1.

[ronag]

I'm not blocking. Just harder to maintain when the spec changes it wording. Then you have to reverse engineering back to the old wording.

[anonrig]

I'm not blocking. Just harder to maintain when the spec changes it wording. Then you have to reverse engineering back to the old wording.

I agree. In my experience, if we followed the spec in Ada word by word, we'll never get this fast.

[KhafraDev]

I think it's important to note the following with this change:

1. no part of the spec is removed or modified: body's source is part of the fetch spec; a step is simply added.
2. the body mixin section of the spec is already not 100% compliant with the spec.
3. undici recommends consuming the body for every single fetch, therefore performance is very important in this area.

Regarding the latter point, there is observable behavior in the formData method in regards to how errors are thrown (either with an invalid this or if the body was already consumed) due to formData adding an extra async tick. fullyReadBody (this step would be skipped with this change) is also a "best-fit" implementation because it's part of the streams spec and as such, requires stream internals. Reading chunks from a readable stream is also known to be slow.

With this change, we are simply skipping over fullyReadBody while implementing the same logic. The spec is unlikely to dramatically change in this area because it would break everything. Even if it did, the change is not invasive and could easily be removed.

[KhafraDev]

I know you're not blocking the change but I typically don't like adding or changing things that are disapproved.

[ronag]

How about -0 then? 🙂