Partially solves perf issues with the UInt8Array checks.#304
Conversation
|
Using |
calvinmetcalf
left a comment
There was a problem hiding this comment.
looks good, do you also want me to update processNextickArgs to add that other case statement ?
bmeurer
left a comment
There was a problem hiding this comment.
As discussed separately, the instanceof check is not semantically equivalent to the type check carried out via O.p.toString, but I suppose it's OKish.
| } | ||
| 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).
| } | ||
| function _isUint8Array(obj) { | ||
| return Object.prototype.toString.call(obj) === '[object Uint8Array]' || Buffer.isBuffer(obj); | ||
| return Buffer.isBuffer(obj) || typeof obj !== 'string' && obj instanceof OurUint8Array; |
| } | ||
| function _isUint8Array(obj) { | ||
| return Object.prototype.toString.call(obj) === '[object Uint8Array]' || Buffer.isBuffer(obj); | ||
| return Buffer.isBuffer(obj) || typeof obj !== 'string' && obj instanceof OurUint8Array; |
| /(?: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