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

Slow read from blob stream #42108

Closed
alanshaw opened this issue Feb 24, 2022 · 1 comment · Fixed by #42117
Closed

Slow read from blob stream #42108

alanshaw opened this issue Feb 24, 2022 · 1 comment · Fixed by #42117
Labels
buffer Issues and PRs related to the buffer subsystem. web streams

Comments

@alanshaw
Copy link

Version

16.14.0

Platform

Darwin Alans-MacBook-Pro.local 21.1.0 Darwin Kernel Version 21.1.0: Wed Oct 13 17:33:01 PDT 2021; root:xnu-8019.41.5~1/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

import crypto from 'crypto'
import { Blob } from 'buffer'

async function main () {
  const bytes = crypto.randomBytes(1024e6)
  const blob = new Blob([bytes])
  const reader = blob.stream().getReader()
  console.time('read')
  const result = await reader.read()
  console.timeEnd('read')
  console.log(result.value.length, 'bytes')
  reader.cancel()
}

main()

Output:

$ node test.js  
read: 203.812ms
65536 bytes

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

Why does it take 203ms to read 65kb from an in memory buffer?

blob.slice(0, 65536) takes 0.148ms so maybe some copying is happening?

const buf = await blob.arrayBuffer()
const slice = buf.slice(0, 65536)

Takes 105ms, which is still half the time stream.read() takes.

Additional information

No response

@alanshaw
Copy link
Author

I looked up what's happening behind the scenes:

node/lib/internal/blob.js

Lines 323 to 339 in 9be1dcf

return new lazyReadableStream({
async start() {
this[kState] = await self.arrayBuffer();
},
pull(controller) {
if (this[kState].byteLength <= kMaxChunkSize) {
controller.enqueue(new Uint8Array(this[kState]));
controller.close();
this[kState] = undefined;
} else {
const slice = this[kState].slice(0, kMaxChunkSize);
this[kState] = this[kState].slice(kMaxChunkSize);
controller.enqueue(new Uint8Array(slice));
}
}
});

@meixg meixg added buffer Issues and PRs related to the buffer subsystem. stream Issues and PRs related to the stream subsystem. labels Feb 24, 2022
@Mesteery Mesteery added web streams and removed stream Issues and PRs related to the stream subsystem. labels Feb 24, 2022
alanshaw pushed a commit to web3-storage/web3.storage that referenced this issue Feb 24, 2022
Node.js v16.14.0 included [a fix](nodejs/node#40706) that meant that `@web-std/blob` started using the Node.js native version.

Bad news, Node.js currently copies the buffer on every iteration when obtaining a stream from `File.stream()`. It also has a fixed and small chunk size of `65536` bytes. This makes reading the stream VERY slow and this test fails because it times out.

I opened an issue about this here: nodejs/node#42108

Once the test was fixed, the cloudflare build for the website started failing so I had to update next.js to v12. WTF!?!

After that, the client build started failing with:

```
Error: Build failed with 2 errors:
../../node_modules/parse-link-header/index.js:3:17: error: Could not resolve "querystring" (use "platform: 'node'" when building for node)
../../node_modules/parse-link-header/index.js:4:18: error: Could not resolve "url" (use "platform: 'node'" when building for node)
    at failureErrorWithLog (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:1493:15)
    at /Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:1151:28
    at runOnEndCallbacks (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:941:63)
    at buildResponseToResult (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:1149:7)
    at /Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:1258:14
    at /Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:629:9
    at handleIncomingPacket (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:726:9)
    at Socket.readFromStdout (/Users/alan/Code/web3-storage/web3.storage/node_modules/esbuild/lib/main.js:596:7)
    at Socket.emit (node:events:520:28)
    at addChunk (node:internal/streams/readable:315:12) {
  errors: [
    {
      detail: undefined,
      location: [Object],
      notes: [],
      pluginName: '',
      text: `Could not resolve "querystring" (use "platform: 'node'" when building for node)`
    },
    {
      detail: undefined,
      location: [Object],
      notes: [],
      pluginName: '',
      text: `Could not resolve "url" (use "platform: 'node'" when building for node)`
    }
  ],
  warnings: []
}
```
So I had to roll the update to `parse-link-header` from #1032 into here as well.
Gozala added a commit to web-std/io that referenced this issue Feb 24, 2022
nodejs-github-bot pushed a commit that referenced this issue Feb 27, 2022
Fix: #42108

PR-URL: #42117
Fixes: #42108
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Mestery <[email protected]>
sxa pushed a commit to sxa/node that referenced this issue Mar 7, 2022
Fix: nodejs#42108

PR-URL: nodejs#42117
Fixes: nodejs#42108
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Mestery <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
Fix: nodejs#42108

PR-URL: nodejs#42117
Fixes: nodejs#42108
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Mestery <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fix: #42108

PR-URL: #42117
Fixes: #42108
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Mestery <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fix: #42108

PR-URL: #42117
Fixes: #42108
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Mestery <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fix: #42108

PR-URL: #42117
Fixes: #42108
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Mestery <[email protected]>
xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
Fix: nodejs#42108

PR-URL: nodejs#42117
Fixes: nodejs#42108
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Mestery <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. web streams
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants