Skip the creation of a transform stream in fetch

[mcollina]

This skips the creation of a TransformStream, which showed up glowing in the following flamegraph:

After this patch:

Benchmarks

main:

[bench:run] ┌─────────┬───────────────────────┬─────────┬────────────────────┬────────────┬─────────────────────────┐
[bench:run] │ (index) │ Tests                 │ Samples │ Result             │ Tolerance  │ Difference with slowest │
[bench:run] ├─────────┼───────────────────────┼─────────┼────────────────────┼────────────┼─────────────────────────┤
[bench:run] │ 0       │ 'undici - fetch'      │ 30      │ '4571.92 req/sec'  │ '± 2.72 %' │ '-'                     │
[bench:run] │ 1       │ 'request'             │ 35      │ '5183.44 req/sec'  │ '± 2.90 %' │ '+ 13.38 %'             │
[bench:run] │ 2       │ 'node-fetch'          │ 10      │ '5405.14 req/sec'  │ '± 1.25 %' │ '+ 18.22 %'             │
[bench:run] │ 3       │ 'axios'               │ 35      │ '5423.29 req/sec'  │ '± 2.80 %' │ '+ 18.62 %'             │
[bench:run] │ 4       │ 'got'                 │ 25      │ '5672.87 req/sec'  │ '± 2.87 %' │ '+ 24.08 %'             │
[bench:run] │ 5       │ 'http - no keepalive' │ 10      │ '6407.75 req/sec'  │ '± 3.00 %' │ '+ 40.15 %'             │
[bench:run] │ 6       │ 'superagent'          │ 10      │ '9827.22 req/sec'  │ '± 1.69 %' │ '+ 114.95 %'            │
[bench:run] │ 7       │ 'http - keepalive'    │ 10      │ '11160.93 req/sec' │ '± 2.49 %' │ '+ 144.12 %'            │
[bench:run] │ 8       │ 'undici - pipeline'   │ 50      │ '12261.22 req/sec' │ '± 3.00 %' │ '+ 168.19 %'            │
[bench:run] │ 9       │ 'undici - stream'     │ 55      │ '19454.69 req/sec' │ '± 2.95 %' │ '+ 325.53 %'            │
[bench:run] │ 10      │ 'undici - request'    │ 20      │ '21013.26 req/sec' │ '± 2.50 %' │ '+ 359.62 %'            │
[bench:run] │ 11      │ 'undici - dispatch'   │ 60      │ '21457.64 req/sec' │ '± 2.93 %' │ '+ 369.34 %'            │
[bench:run] └─────────┴───────────────────────┴─────────┴────────────────────┴────────────┴─────────────────────────┘

this pr:

[bench:run] ┌─────────┬───────────────────────┬─────────┬────────────────────┬────────────┬─────────────────────────┐
[bench:run] │ (index) │ Tests                 │ Samples │ Result             │ Tolerance  │ Difference with slowest │
[bench:run] ├─────────┼───────────────────────┼─────────┼────────────────────┼────────────┼─────────────────────────┤
[bench:run] │ 0       │ 'undici - fetch'      │ 50      │ '5056.83 req/sec'  │ '± 2.78 %' │ '-'                     │
[bench:run] │ 1       │ 'axios'               │ 30      │ '5253.49 req/sec'  │ '± 2.92 %' │ '+ 3.89 %'              │
[bench:run] │ 2       │ 'node-fetch'          │ 10      │ '5467.21 req/sec'  │ '± 1.25 %' │ '+ 8.12 %'              │
[bench:run] │ 3       │ 'request'             │ 35      │ '5501.59 req/sec'  │ '± 2.92 %' │ '+ 8.80 %'              │
[bench:run] │ 4       │ 'got'                 │ 30      │ '5758.56 req/sec'  │ '± 2.67 %' │ '+ 13.88 %'             │
[bench:run] │ 5       │ 'http - no keepalive' │ 15      │ '6173.06 req/sec'  │ '± 2.81 %' │ '+ 22.07 %'             │
[bench:run] │ 6       │ 'superagent'          │ 10      │ '9717.62 req/sec'  │ '± 1.60 %' │ '+ 92.17 %'             │
[bench:run] │ 7       │ 'http - keepalive'    │ 10      │ '11309.90 req/sec' │ '± 2.53 %' │ '+ 123.66 %'            │
[bench:run] │ 8       │ 'undici - pipeline'   │ 50      │ '11740.51 req/sec' │ '± 2.95 %' │ '+ 132.17 %'            │
[bench:run] │ 9       │ 'undici - request'    │ 80      │ '17345.44 req/sec' │ '± 2.89 %' │ '+ 243.01 %'            │
[bench:run] │ 10      │ 'undici - stream'     │ 70      │ '20085.77 req/sec' │ '± 2.93 %' │ '+ 297.20 %'            │
[bench:run] │ 11      │ 'undici - dispatch'   │ 60      │ '21427.77 req/sec' │ '± 2.97 %' │ '+ 323.74 %'            │
[bench:run] └─────────┴───────────────────────┴─────────┴────────────────────┴────────────┴─────────────────────────┘

It improves our benchmark by 10%.

It also shortens the following:

import { fetch } from './index.js'

console.time('fetch')
for (let i = 0; i < 100000; i++) {
  const res = await fetch('http://localhost:3000')
  // console.log(await res.text())
  await res.text()
}
console.timeEnd('fetch')

from 20s to 16s. A nice 20% speedup.

[ronag]

Can we ask the spec people why this is there?

[mcollina]

From the spec:

This TransformStream is needed for the purpose of receiving a notification when the stream reaches its end, and is otherwise an identity transform stream.

which we fulfill in https://github.com/nodejs/undici/pull/3093/files#diff-9cc53c7d17eaefd294df4a58f7406660dfe9e2b39feea783e824d33095cafedeR1106.

---

So in other terms, it seems they had no easy way to specify what we did in our implementation.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.03%. Comparing base (2e128c1) to head (db3594d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3093      +/-   ##
==========================================
- Coverage   94.03%   94.03%   -0.01%     
==========================================
  Files          89       89              
  Lines       24357    24356       -1     
==========================================
- Hits        22904    22903       -1     
  Misses       1453     1453              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KhafraDev]

We definitely have a bug regarding internalResponse.body.stream, this can land in the meantime.

[KhafraDev]

We can almost get rid of webstreams here entirely (only 4 WPTs fail):

diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js
index 30f457df..21a7f0ae 100644
--- a/lib/web/fetch/index.js
+++ b/lib/web/fetch/index.js
@@ -1089,32 +1089,9 @@ function fetchFinale (fetchParams, response) {
     //    set to processResponseEndOfBody.
     // 4. Set internalResponse’s body’s stream to the result of internalResponse’s body’s stream piped through transformStream.

-    const byteStream = new ReadableStream({
-      readableStream: internalResponse.body.stream,
-      async start () {
-        this._bodyReader = this.readableStream.getReader()
-      },
-      async pull (controller) {
-        while (controller.desiredSize >= 0) {
-          const { done, value } = await this._bodyReader.read()
-
-          if (done) {
-            queueMicrotask(() => {
-              readableStreamClose(controller)
-
-              // Added to in lieu of steps 1-4 of the spec
-              processResponseEndOfBody()
-            })
-            break
-          }
-
-          controller.enqueue(value)
-        }
-      },
-      type: 'bytes'
+    require('node:stream').finished(internalResponse.body.stream, (err) => {
+      processResponseEndOfBody()
     })
-
-    internalResponse.body.stream = byteStream
   }
 }

[mcollina]

I didn't want to lower the WPT coverage. PR for that change?

[KhafraDev]

#3095, I managed to fix the failures

[KhafraDev]

With the benchmarks, it was ~3.5% faster, but that's only the difference between a couple of promise allocations (from the stream) and a single one from stream.finished. The larger the body, the bigger the gains should be.