search

nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 227 forks source link

Partially solves perf issues with the UInt8Array checks. #304

Closed mcollina closed 7 years ago

mcollina commented 7 years ago

Fixes https://github.com/nodejs/readable-stream/issues/302

This is a partial fix.

Node 6.11, using readable-stream 2.2.11:

benchThrough2*10000: 5148.149ms

Node 6.11, using readable-stream 2.3.2:

benchThrough2*10000: 18112.640ms

Node 6.11, using this patch:

benchThrough2*10000: 6491.796ms

I think we can do better.

cc @bmeurer

mcollina commented 7 years ago

Using instanceof results in a better:

benchThrough2*10000: 5649.387ms
mcollina commented 7 years ago

@bmeurer thanks, updated.

kgryte commented 7 years ago

The move to using instanceof will fail for cross-realm Uint8Array objects (e.g., from iframes or, depending on the setup/Node.js version, different node V8 vm's). Is that a concern?

In contrast, the Object.prototype.toString() approach supports cross-realm objects.

mcollina commented 7 years ago

I do not think so. Considering the cost of using the toString() approach, I think it's a trade-off that we must accept.

bmeurer commented 7 years ago

Ideally there'd be an instance type check for Uint8Array and friends. instanceof doesn't really check whether the object is an Uint8Array, but rather if it has a certain prototype, which is obviously realm specific.