Partially solves perf issues with the UInt8Array checks. #304
Conversation
|
Using |
build/files.js
Outdated
| function _uint8ArrayToBuffer(chunk) { | ||
| return Buffer.from(chunk); | ||
| } | ||
| function _isUint8Array(obj) { | ||
| return Object.prototype.toString.call(obj) === '[object Uint8Array]' || Buffer.isBuffer(obj); | ||
| return Buffer.isBuffer(obj) || (typeof obj !== 'string' && obj instanceof OurUint8Array); |
There was a problem hiding this comment.
Why not typeof obj === 'object'?
There was a problem hiding this comment.
Actually now that I think about it, the typeof check is just unnecessary complexity. instanceof itself already does a type check anyways (as one of the first steps).
lib/_stream_readable.js
Outdated
| function _uint8ArrayToBuffer(chunk) { | ||
| return Buffer.from(chunk); | ||
| } | ||
| function _isUint8Array(obj) { | ||
| return Object.prototype.toString.call(obj) === '[object Uint8Array]' || Buffer.isBuffer(obj); | ||
| return Buffer.isBuffer(obj) || typeof obj !== 'string' && obj instanceof OurUint8Array; |
lib/_stream_writable.js
Outdated
| function _uint8ArrayToBuffer(chunk) { | ||
| return Buffer.from(chunk); | ||
| } | ||
| function _isUint8Array(obj) { | ||
| return Object.prototype.toString.call(obj) === '[object Uint8Array]' || Buffer.isBuffer(obj); | ||
| return Buffer.isBuffer(obj) || typeof obj !== 'string' && obj instanceof OurUint8Array; |
| @@ -180,11 +180,12 @@ const headRegexp = /(^module.exports = \w+;?)/m | |||
| /(?:var|const) Buffer = require\('buffer'\)\.Buffer;/ | |||
| , `/*<replacement>*/ | |||
| const Buffer = require('safe-buffer').Buffer | |||
| const OurUint8Array = global.Uint8Array || function () {} | |||
There was a problem hiding this comment.
Another thing: Use var instead of const here (and in the other files below), to ensure that the instanceof optimization definitely triggers.
There was a problem hiding this comment.
we are passing through babel anyway, these are all vars.
|
@bmeurer thanks, updated. |
|
The move to using In contrast, the |
|
I do not think so. Considering the cost of using the |
|
Ideally there'd be an instance type check for |
Fixes #302
This is a partial fix.
Node 6.11, using readable-stream 2.2.11:
Node 6.11, using readable-stream 2.3.2:
Node 6.11, using this patch:
I think we can do better.
cc @bmeurer