stream: use instanceof when checking chunk type

[lpinca]

Use the instanceof operator instead of Stream._isUint8Array() when
checking if a chunk is a Uint8Array as this is faster.

'use strict';constassert=require('assert');constBenchmark=require('benchmark');const{ _isUint8Array }=require('stream');constsuite=newBenchmark.Suite();constbuf=Buffer.alloc(0);varret;suite.add('instanceof',function(){ret=bufinstanceofUint8Array;assert(ret);});suite.add('_isUint8Array',function(){ret=_isUint8Array(buf);assert(ret);});suite.on('cycle',function(event){console.log(event.target.toString());});suite.on('complete',function(){console.log('Fastest is '+this.filter('fastest').map('name'));});suite.run({async: true});

Node.js 9.11.1:

$ node index.js instanceof x 151,468,107 ops/sec ±0.31% (90 runs sampled)
_isUint8Array x 66,015,037 ops/sec ±0.28% (89 runs sampled)
Fastest is instanceof

Node.js master:

$ ../node/node index.js instanceof x 217,093,783 ops/sec ±0.14% (94 runs sampled)
_isUint8Array x 179,445,229 ops/sec ±0.22% (93 runs sampled)
Fastest is instanceof

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[lpinca]

CI: https://ci.nodejs.org/job/node-test-pull-request/14123/

[devsnek]

i feel compelled to mention util.types.isUint8Array, i assume we want to start making our type checks consistent

[lpinca]

@devsnekStream._isUint8Array === util.types.isUint8Array

[devsnek]

@lpinca why not then change util.types.isUint8Array to use instanceof

[lpinca]

Yes it makes sense.

[anliting]

It seems that isUint8Array have a different semantic comparing to instanceof:

// start: copied from /lib/internal/util/types.jsconstReflectApply=Reflect.apply;functionuncurryThis(func){return(thisArg, ...args)=>ReflectApply(func,thisArg,args);}constTypedArrayPrototype=Object.getPrototypeOf(Uint8Array.prototype);constTypedArrayProto_toStringTag=uncurryThis(Object.getOwnPropertyDescriptor(TypedArrayPrototype,Symbol.toStringTag).get);functionisUint8Array(value){returnTypedArrayProto_toStringTag(value)==='Uint8Array';}// end: copied from /lib/internal/util/types.jsleta={__proto__:Uint8Array.prototype}console.log(ainstanceofUint8Array,// trueisUint8Array(a)// false)

Also see (Uint8Array might also in the same case): https://stackoverflow.com/a/22289982

[lpinca]

@anliting good point. That's a good reason to not change util.types functions. I don't know if we need to be so strict on streams. In v6.x there was no support for Uint8Array in streams and we were using instanceof to check if a chunk was a buffer.

[addaleax]

This might potentially be breaking because instanceof does something else than util.types.isUint8Array, as noted above.

If you are looking for a faster alternative, did you try chunk[Symbol.toStringTag] === 'Uint8Array'? That is not fool-proof either, but I could imagine that the engine could handle it pretty well.

why not then change util.types.isUint8Array to use instanceof

That defeats the purpose of having these functions – the util.types checks are supposed something that works in a cross-context manner and isn’t fooled by modifying prototypes in some way.

[lpinca]

If you are looking for a faster alternative, did you try chunk[Symbol.toStringTag] === 'Uint8Array'? That is not fool-proof either, but I could imagine that the engine could handle it pretty well.

It has the same performance of util.types.isUint8Array() and as you says it's not better than instanceof:

constassert=require('assert');classFoo{get[Symbol.toStringTag](){return'Uint8Array';}}constfoo=newFoo();assert(foo[Symbol.toStringTag]==='Uint8Array');

[BridgeAR]

In general: I am a fan of having strict type checks. In this specific case, I believe it will not make a difference besides the performance gain, so I am +1.

@addaleax

This might potentially be breaking because instanceof does something else than util.types.isUint8Array [...]

The first check does not seem to be breaking, because it will just allow weird chunks to be accepted instead of throwing an error. The second entry seems different though and it could be counted as breaking. But why would someone want to try to pass in a manipulated chunk? It would only be bad for the user in that case, so I do not see an issue there either.

[TimothyGu]

The compatibility issue at play is that this will no longer accept Uint8Array objects from other contexts. I am not convinced this is a good idea. It’s not that this new check is “not as strict,” but rather “not as correct.”

I would also be very much surprised if the result from this benchmark affects any real-world applications of Node.js.

[TimothyGu]

I will also note that V8 has been willing to do more optimizations on %TypedArrayPrototype%[\@\@toStringTag], as seen in #15663 and https://crbug.com/v8/6874.

[BridgeAR]

@TimothyGu thanks a lot for bringing up the context. I did not think about that anymore.

Removing my LG due to the context part.

[BridgeAR]

@TimothyGu

I will also note that V8 has been willing to do more optimizations on %TypedArrayPrototype%[@@toStringTag]

The referenced issue is marked as fixed. So master should already reflect that performance boost with the toStringTag.

[lpinca]

The referenced issue is marked as fixed. So master should already reflect that performance boost with the toStringTag.

Yes, it is no longer 2/3x slower but still not as fast as instanceof as reported by the above benchmark.
WRT to the context issue, doesn't it also apply for Buffer.isBuffer() and all other places where instanceof is already used?

Also please take a look at #19559 which is the reason why I would like to make chunkInvalid() as fast as possible.

[addaleax]

WRT to the context issue, does it also apply for Buffer.isBuffer() and all other places where instanceof is already used?

Yes, it’s a general issue with instanceof – foo instanceof Bar checks the prototype chain of foo for Bar.prototype, and different contexts generally will have different Bar.prototype objects for built-in Bars.

Also, by the way: We did move away from instanceof in quite a few places, because it’s such a complex operation and usually there were more performant alternatives than that.

If you are looking for a faster alternative, did you try chunk[Symbol.toStringTag] === 'Uint8Array'? That is not fool-proof either, but I could imagine that the engine could handle it pretty well.

It has the same performance of util.types.isUint8Array() and as you says it's not better than instanceof:

It can be fooled, but if it is, I think we can assume that it’s intentional – and the approach does work in a multi-context way.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/153/

[lpinca]

@addaleax yes my point was that we still have the context issue in other places in particular with Buffer.isBuffer() and this is a known "issue". Is it common to move data chunk in streams between different contexts and have we had issues opened for this reason in Node.js <= 6?

Fooling instanceof is also intentional imho. If there is something safer and faster I am all for it obviously :)

[addaleax]

Is it common to move data chunk in streams between different contexts and have we had issues opened for this reason in Node.js <= 6?

Not really, I guess.

Fooling instanceof is also intentional imho. If there is something safer and faster I am all for it obviously :)

I don’t know, tbh. Fwiw, here’s the PR that originally moved away from instanceof, including the benchmarks that showed a significant performance increase at the time: #11608

[lpinca]

I remember, instanceof was very slow just like Function.prototype.bind() which is now fast! It's hard to follow VMs changes.

[BridgeAR]

We did move away from instanceof in quite a few places, because it’s such a complex operation and usually there were more performant alternatives than that.

With Turbofan instanceof is by far not that bad anymore. It is still slower than e.g. typeof but not even close as bad as it used to be.

[addaleax]

@lpinca How did you benchmark this? The benchmark CI shows no significant results for streams (positive or negative), which is kind of surprising to me.

[lpinca]

@addaleax benchmark source is in PR description. You started it on master where the impact is low.
Here is a run against v8.x: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/154/console
Not a huge improvement but still relevant.

[addaleax]

@lpinca I see – I’m not sure then what to think. I would probably consider this a breaking change, but it seems like the places where we’d actually want it are v8.x and v9.x?

[lpinca]

Yes, if it can't be backported it doesn't make sense. it's a ~6% improvement on read and ~6% on write, not bad for duplex streams, multiply that for thousands of streams and the gain is not insignificant imho.

There are no explicit -1 yet but it seems people are inclined toward that. If anyone makes it explicit I'll close this.

[BridgeAR]

I have no strong opinion about this and would like to defer this to @addaleax

[lpinca]

No approvals or disapprovals, this is going nowhere, closing.