Partially solves perf issues with the UInt8Array checks.#304
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mcollina commented Jun 27, 2017
Using |
calvinmetcalf left a comment
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
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.
build/files.js Outdated
| } | ||
| 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.
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_isUint8Array(obj){ | ||
| returnObject.prototype.toString.call(obj)==='[object Uint8Array]'||Buffer.isBuffer(obj); | ||
| returnBuffer.isBuffer(obj)||typeofobj!=='string'&&objinstanceofOurUint8Array; |
lib/_stream_writable.js Outdated
| } | ||
| function_isUint8Array(obj){ | ||
| returnObject.prototype.toString.call(obj)==='[object Uint8Array]'||Buffer.isBuffer(obj); | ||
| returnBuffer.isBuffer(obj)||typeofobj!=='string'&&objinstanceofOurUint8Array; |
| /(?: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.
mcollina commented Jun 28, 2017
@bmeurer thanks, updated. |
kgryte commented Jun 28, 2017
The move to using In contrast, the |
mcollina commented Jun 28, 2017
I do not think so. Considering the cost of using the |
bmeurer commented Jun 28, 2017
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