lib: faster type checks for some types by TimothyGu · Pull Request #15663 · nodejs/node

TimothyGu · 2017-09-28T19:02:24Z

The JS<->C++ overhead is fairly high with TurboFan. If there is a way to do a reliable and performant type check in JS, use JS instead.

This PR introduces a new internal module that also protects against modification of the global environment by caching all utilized functions at Node.js startup.

 improvement confidence p.value
buffers/buffer-compare.js millions=30 size=0 23.84 % *** 1.301551e-11
buffers/buffer-compare.js millions=30 size=32 23.22 % *** 5.364324e-13
buffers/buffer-compare.js millions=10 size=4096 10.57 % *** 2.771584e-13

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

lib

BridgeAR · 2017-09-28T21:05:26Z

The args will always be a empty array. So you might just use a empty array that you pass in for the argumentsList instead of creating a new array each time?

The spread syntax is actually faster than hardcoding an empty array by a good amount. Probably because of the fact that Reflect.apply is inlined.

Interesting and good to know. In that case a comment about that would be good as well.

@BridgeAR comment added.

BridgeAR · 2017-09-28T21:06:13Z

Would you be so kind and add a comment that you added this to make sure it will not be manipulated by userland code?

cjihrig · 2017-09-29T01:23:28Z

Having some of these type checking functions in util, and others in this new location isn't very intuitive IMO. I think we should consolidate in one of those locations.

TimothyGu · 2017-09-29T01:36:41Z

@cjihrig The difference is that the type checkers in util are both public and deprecated.

cjihrig · 2017-09-29T02:17:55Z

Right now we have type checking functions in util, internal/util, and process.binding('util'). Regardless of their status, at least the common theme of util makes sense.

TimothyGu · 2017-09-29T02:24:10Z

@cjihrig Would internal/util/types work for you? internal/util has too much stuff unrelated to type checking, and I'm not sure why the only type checker (isError) in that file is there because it's exposed through util anyway.

cjihrig · 2017-09-29T02:32:49Z

That works for me.

bmeurer · 2017-09-29T07:52:46Z

So I'm not entirely sure what is the scope of this change. Is it because ArrayBuffer.isView is not inlined into TurboFan? If so, that would be easy to fix in V8.

TimothyGu · 2017-09-29T08:19:57Z

So I'm not entirely sure what is the scope of this change. Is it because ArrayBuffer.isView is not inlined into TurboFan? If so, that would be easy to fix in V8.

No; this PR has two parts:

Use a faster version of some type checks (in the case of TypedArray and Uint8Array), and
Prevent user code tampering with globals from affecting Node.js core by caching the untampered functions.

The change about ArrayBuffer.isView is of the latter ilk only. I'm 100% happy with the performance of it (though if you can improve it further then go ahead)!

bmeurer · 2017-09-29T08:25:15Z

Ah, right, just discovered that. The isArrayBufferView part was confusing me. I'll dig into ArrayBuffer.isView to speed that up a bit.

lpinca · 2017-09-29T08:28:25Z

The isArrayBufferView part was confusing me.

Yes, I agree. No strong opinions, but maybe it's better to move that part to its own pr or commit.

bmeurer · 2017-09-29T09:12:07Z

Ok, I have a CL in flight that improves ArrayBuffer.isView by roughly 2.5x, and should allow for more optimizations in the surrounding code.

TimothyGu · 2017-09-30T07:23:09Z

@bmeurer If possible, please optimize %TypedArray%.prototype[Symbol.toStringTag]'s getter as well. I'm using it quite heavily in this PR. Thanks 😊

TimothyGu · 2017-09-30T07:25:46Z

@cjihrig Can you check if this version is acceptable for you? I've renamed it to internal/util/types

bmeurer

@TimothyGu At this point, TypedArray.prototype[Symbol.toStringTag] is not really fast in V8. It's also not super-slow. I'll try to get that fixed and inlined next week.

As for this particular PR, I'm not sure uncurryThis is really optimized fully with V8 6.1.

bmeurer · 2017-09-30T10:54:19Z

Have you checked the generated code for isUint8Array? I don't think we get the Reflect.apply optimization with V8 6.1 already.

Yes I have:
--- Raw source ---(value) { return TypedArrayProto_toStringTag(value) === 'Uint8Array';}--- Optimized code ---optimization_id = 0source_position = 1086kind = OPTIMIZED_FUNCTIONname = isUint8Arraystack_slots = 4compiler = turbofanInstructions (size = 194)0x367169e055c0055pushrbp0x367169e055c11 4889e5 REX.W movqrbp,rsp0x367169e055c4456pushrsi0x367169e055c5557pushrdi0x367169e055c66 493ba5e80c0000 REX.W cmpq rsp,[r13+0xce8]0x367169e055cd d 0f8656000000 jna0x367169e05629 <+0x69>0x367169e055d313 48bf59b6c858e83c0000 REX.W movqrdi,0x3ce858c8b659 ;; object: 0x3ce858c8b659 <JSFunction b (sfi = 0x2fc5e3493879)>0x367169e055dd 1d 488b7727 REX.W movqrsi,[rdi+0x27]0x367169e055e121 488b5d10 REX.W movqrbx,[rbp+0x10]0x367169e055e52553pushrbx0x367169e055e626 498b55a0 REX.W movqrdx,[r13-0x60]0x367169e055ea 2a 33c0 xorl rax,rax0x367169e055ec 2c ff5737 call[rdi+0x37]0x367169e055ef 2f a801 testal,0x10x367169e055f131 0f844e000000 jz0x367169e05645 <+0x85>0x367169e055f737 488b58ff REX.W movqrbx,[rax-0x1]0x367169e055fb 3b f6430bc0 testb [rbx+0xb],0xc00x367169e055ff 3f 0f8545000000 jnz0x367169e0564a <+0x8a>0x367169e0560545 48bb711549e3c52f0000 REX.W movqrbx,0x2fc5e3491571 ;; object: 0x2fc5e3491571 <String[10]: Uint8Array>0x367169e0560f 4f 483bd8 REX.W cmpq rbx,rax0x367169e0561252 0f840b000000 jz0x367169e05623 <+0x63>0x367169e0561858 498b45c0 REX.W movqrax,[r13-0x40]0x367169e0561c 5c 488be5 REX.W movqrsp,rbp0x367169e0561f 5f 5d poprbp0x367169e0562060 c21000 ret0x100x367169e0562363 498b45b8 REX.W movqrax,[r13-0x48]0x367169e0562767 ebf3 jmp0x367169e0561c <+0x5c>0x367169e0562969 48bb1067f50000000000 REX.W movqrbx,0xf567100x367169e0563373 33c0 xorl rax,rax0x367169e0563575 488b75f8 REX.W movqrsi,[rbp-0x8]0x367169e0563979 e8a2efe7ff call0x367169c845e0 ;; code: STUB, CEntryStub, minor: 80x367169e0563e 7e eb93 jmp0x367169e055d3 <+0x13>0x367169e0564080 e8bbe9cfff call0x367169b04000 ;; deoptimization bailout 00x367169e0564585 e8c0e9cfff call0x367169b0400a ;; deoptimization bailout 10x367169e0564a 8a e8c5e9cfff call0x367169b04014 ;; deoptimization bailout 20x367169e0564f 8f e8cae9cfff call0x367169b0401e ;; deoptimization bailout 30x367169e056549490nop0x367169e056559590nop...Source positions: pc offset position26553691086Inlined functions (count = 1)0x3ce858ce6719 <SharedFunctionInfo args>Deoptimization Input Data (deopt points = 4) index bytecode-offset trampoline_pc pc02880 2f 11585 NA 215 8a NA 30 8f 7e Safepoints (size = 30)0x367169e055ef 2f 0000 (sp-> fp) 00x367169e0563e 7e 0000 (sp-> fp) 3RelocInfo (size = 11)0x367169e055d5 embedded object (0x3ce858c8b659 <JSFunction b (sfi = 0x2fc5e3493879)>)0x367169e05607 embedded object (0x2fc5e3491571 <String[10]: Uint8Array>)0x367169e0563a code target (STUB) (0x367169c845e0)0x367169e05641 runtime entry (deoptimization bailout 0)0x367169e05646 runtime entry (deoptimization bailout 1)0x367169e0564b runtime entry (deoptimization bailout 2)0x367169e05650 runtime entry (deoptimization bailout 3)--- End code ---
Note: function b() is in fact %TypedArray%.prototype.toStringTag, and args is the name given to the wrapper function created in uncurryThis, which as you can tell is fully inlined.
Note 2: I don't really care if Reflect.apply is fully optimized for now, because regardless of that this function is much faster than the check we are currently using (#15663 (comment)).

Ok, with more recent V8 this will get even better (I also have a patch to inline TypedArray.prototype[@@toStringTag]). Nevertheless, why don't we just use F.p.call here and avoid uncurryThis completely?

@bmeurerFunction.prototype.call can be overwritten by userland code. I don't want that possibility for a type checking function.

I see. Makes sense.

bmeurer · 2017-09-30T10:55:03Z

Nit: This took me some time to understand. Maybe just a comment would help, or inline the getterFunc to make it obvious what is being done.

Okay. Done.

This improves performance of ArrayBuffer.isView by roughly 2.5x itself, and enables optimizations across ArrayBuffer.isView calls, i.e. map checks can be eliminated across. This was discovered in a related Node pull request (nodejs/node#15663). Bug: v8:6868 Change-Id: I1d56ec385f8daa0e1d44d3bc4d6c9a5558ba4522 Reviewed-on: https://chromium-review.googlesource.com/691660 Reviewed-by: Yang Guo <yangguo@chromium.org> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org> Cr-Commit-Position: refs/heads/master@{#48247}

TimothyGu · 2017-09-30T18:19:23Z

@bmeurer thanks for taking a look!

At this point, TypedArray.prototype[Symbol.toStringTag] is not really fast in V8. It's also not super-slow.

Yeah I realized. The %_IsTypedArray check is inlined which is pretty nice, but %_ClassOf calls into the runtime which isn't as nice, but still not as slow as it was before.

BridgeAR · 2017-10-01T22:52:26Z

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

BridgeAR · 2017-10-02T02:40:31Z

Landed in 7907534 and f547db1

The TypedArray.prototype[Symbol.toStringTag] getter is currently the best (and as far as I can tell only definitely side-effect free) way to check whether an arbitrary object is a TypedArray - either generally TypedArray or a specific one like Uint8Array. Using the getter is thus emerging as the general pattern to detect TypedArrays, even Node.js now adapted it starting with nodejs/node#15663 for the isTypedArray and isUint8Array type checks in lib/internal/util/types.js now. The getter returns either the string with the TypedArray subclass name (i.e. "Uint8Array") or undefined if the receiver is not a TypedArray. This can be implemented with a simple elements kind dispatch, instead of checking the instance type and then loading the class name from the constructor, which requires a loop walking up the transition tree. This CL ports the builtin to CSA and TurboFan, and changes the logic to a simple elements kind check. On the micro-benchmark mentioned in the referenced bug, the time goes from testIsArrayBufferView: 565 ms. testIsTypedArray: 2403 ms. testIsUint8Array: 3847 ms. to testIsArrayBufferView: 566 ms. testIsTypedArray: 965 ms. testIsUint8Array: 965 ms. which presents an up to 4x improvement. Bug: v8:6874 Change-Id: I9c330b4529d9631df2f052acf023c6a4fae69611 Reviewed-on: https://chromium-review.googlesource.com/695021 Reviewed-by: Jaroslav Sevcik <jarin@chromium.org> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org> Cr-Commit-Position: refs/heads/master@{#48254}

bmeurer · 2017-10-02T07:25:25Z

V8 optimization for TypedArray.prototype[Symbol.toStringTag] is v8:6874.

MylesBorins · 2017-10-03T17:53:17Z

This does not land cleanly on v8.x, could you please backport

PR-URL: nodejs/node#15663 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: nodejs#16073 PR-URL: nodejs#15663 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #16073 PR-URL: #15663 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-11-16T20:24:43Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

This function was added by nodejs#15663, but was never documented. This commit documents it.

This function was added by #15663, but was never documented. This commit documents it. PR-URL: #33092 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Masashi Hirano <shisama07@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 28, 2017

addaleax approved these changes Sep 28, 2017
View reviewed changes

BridgeAR approved these changes Sep 28, 2017
View reviewed changes

mscdex added the performance Issues and PRs related to the performance of Node.js. label Sep 28, 2017

lpinca approved these changes Sep 29, 2017
View reviewed changes

TimothyGu force-pushed the is-uint8array branch from bb151db to 8d5d39dCompare September 30, 2017 07:24

bmeurer suggested changes Sep 30, 2017
View reviewed changes

bmeurer approved these changes Sep 30, 2017
View reviewed changes

TimothyGu added 2 commits September 30, 2017 11:20

lib: faster type checks for some types
e1f939a

benchmark: add util/type-check
6116c39

TimothyGu force-pushed the is-uint8array branch from 3a63918 to 6116c39Compare September 30, 2017 18:24

cjihrig approved these changes Oct 1, 2017
View reviewed changes

jasnell approved these changes Oct 1, 2017
View reviewed changes

BridgeAR closed this Oct 2, 2017

TimothyGu deleted the is-uint8array branch October 2, 2017 08:24

MylesBorins added the backport-requested-v8.x label Oct 3, 2017

TimothyGu mentioned this pull request Oct 7, 2017
[v8.x backport] lib: faster type checks for some types #16073
Closed
2 tasks

lance added backported-to-v8.x and removed backport-requested-v8.x labels Oct 28, 2017

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

TimothyGu added dont-land-on-v6.x and removed backport-requested-v6.x labels Nov 16, 2017

TimothyGu mentioned this pull request Apr 8, 2018
stream: use instanceof when checking chunk type #19872
Closed
2 tasks

kevinoid mentioned this pull request Apr 27, 2020
doc: add util.types.isArrayBufferView() #33092
Closed
2 tasks

kevinoid added a commit to kevinoid/node that referenced this pull request Apr 27, 2020
doc: add util.types.isArrayBufferView()
cb8f51e
This function was added by nodejs#15663, but was never documented. This commit documents it.

Uh oh!

Uh oh!

Conversation

TimothyGu commented Sep 28, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeARSep 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Sep 29, 2017

Uh oh!

TimothyGu commented Sep 29, 2017

Uh oh!

cjihrig commented Sep 29, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Sep 29, 2017

Uh oh!

cjihrig commented Sep 29, 2017

Uh oh!

bmeurer commented Sep 29, 2017

Uh oh!

TimothyGu commented Sep 29, 2017

Uh oh!

bmeurer commented Sep 29, 2017

Uh oh!

lpinca commented Sep 29, 2017

Uh oh!

bmeurer commented Sep 29, 2017

Uh oh!

TimothyGu commented Sep 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Sep 30, 2017

Uh oh!

bmeurer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmeurerSep 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGuSep 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Sep 30, 2017

Uh oh!

BridgeAR commented Oct 1, 2017

Uh oh!

BridgeAR commented Oct 2, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeurer commented Oct 2, 2017

Uh oh!

MylesBorins commented Oct 3, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

BridgeARSep 28, 2017•
edited
Loading

cjihrig commented Sep 29, 2017•
edited
Loading

TimothyGu commented Sep 30, 2017•
edited
Loading

bmeurerSep 30, 2017•
edited
Loading

TimothyGuSep 30, 2017•
edited
Loading

BridgeAR commented Oct 2, 2017•
edited
Loading