streams: Ensure that instanceof fast-path is hit. #13403

bmeurer · 2017-06-02T14:56:59Z

With the new Ignition+TurboFan pipeline, the instanceof fast-path can be
missed if the right-hand side needs a TDZ check, i.e. is const declared
on a surrounding scope. This doesn't apply to Node 8 at this point,
where it's at V8 5.8, but it applies as soon as V8 5.9 rolls. There's
work going on in Ignition (and TurboFan) to optimize those TDZ checks
properly, but those changes will land in V8 6.1, so might not end up in
Node 8.

One way to work-around this in Node core for now is to use var instead
of const for those right-hand sides for instanceof for now, especially
Buffer in case of streams. This is not beautiful, but proper ducktape.
Improves readable-bigread.js by ~23% with Node LKGR.

Checklist

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

Affected core subsystem(s)

mcollina

LGTM

mcollina · 2017-06-02T15:01:22Z

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

With the new Ignition+TurboFan pipeline, the instanceof fast-path can be missed if the right-hand side needs a TDZ check, i.e. is const declared on a surrounding scope. This doesn't apply to Node 8 at this point, where it's at V8 5.8, but it applies as soon as V8 5.9 rolls. There's work going on in Ignition (and TurboFan) to optimize those TDZ checks properly, but those changes will land in V8 6.1, so might not end up in Node 8. One way to work-around this in Node core for now is to use var instead of const for those right-hand sides for instanceof for now, especially Buffer in case of streams. This is not beautiful, but proper ducktape. Improves readable-bigread.js by ~23% with Node LKGR.

mscdex · 2017-06-02T15:25:59Z

Last CI had some issue checking out the PR on every platform. Here's a new CI: https://ci.nodejs.org/job/node-test-pull-request/8438/

jasnell · 2017-06-02T18:01:04Z

bleh... not a fan of this but ok. How long should we expect this to be necessary?

mscdex · 2017-06-02T18:04:53Z

It may not be desirable, but even if/when node v8.x gets the proper const fix, having this backported would probably make existing LTS users happy, assuming this change is applicable to those older V8 versions.

silverwind · 2017-06-02T18:30:50Z

Might be good to also update the lint rule advice for that exact line:

node/tools/eslint-rules/require-buffer.js

Line 5 in 7b56fd8

const msg = 'Use const Buffer = require(\'buffer\').Buffer; ' +

Are these the only two cases of const Buffer?

mscdex · 2017-06-02T18:33:44Z

I'm wondering if we should be doing this for many other instances of instanceof throughout node (mainly the ones used in hot paths) as well, Buffer or otherwise? If we did it all in a single commit it would be easier to revert later on in master.

bmeurer · 2017-06-02T21:15:22Z

@jasnell Work is currently going on in Ignition (and TurboFan) as said, i.e. part of that is in https://chromium-review.googlesource.com/c/509613/, which is in review.

bmeurer · 2017-06-02T21:40:48Z

Not sure you've seen https://twitter.com/jsumners79/status/870753471481434112, which suggests to use O.p.isPrototypeOf. I think this is currently not necessarily faster in V8, but sounds like a good long-term strategy, as it avoids the potential instanceof performance cliff. I can look into making it fast in V8.

addaleax · 2017-06-09T19:45:07Z

Landed in 55f9c85

With the new Ignition+TurboFan pipeline, the instanceof fast-path can be missed if the right-hand side needs a TDZ check, i.e. is const declared on a surrounding scope. This doesn't apply to Node 8 at this point, where it's at V8 5.8, but it applies as soon as V8 5.9 rolls. There's work going on in Ignition (and TurboFan) to optimize those TDZ checks properly, but those changes will land in V8 6.1, so might not end up in Node 8. One way to work-around this in Node core for now is to use var instead of const for those right-hand sides for instanceof for now, especially Buffer in case of streams. This is not beautiful, but proper ducktape. Improves readable-bigread.js by ~23% with Node LKGR. PR-URL: #13403 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Calvin Metcalf <[email protected]>

Port the baseline implementation of Object.prototype.isPrototypeOf to the CodeStubAssembler, sharing the existing prototype chain lookup logic with the instanceof / OrdinaryHasInstance implementation. Based on that, do the same in TurboFan, introducing a new JSHasInPrototypeChain operator, which encapsulates the central prototype chain walk logic. This speeds up Object.prototype.isPrototypeOf by more than a factor of four, so that the code A.prototype.isPrototypeOf(a) is now performance-wise on par with a instanceof A for the case where A is a regular constructor function and a is an instance of A. Since instanceof does more than just the fundamental prototype chain lookup, it was discovered in Node core that O.p.isPrototypeOf would be a more appropriate alternative for certain sanity checks, since it's less vulnerable to monkey-patching. In addition, the Object builtin would also avoid the performance-cliff associated with instanceof (due to the Symbol.hasInstance hook), as for example hit by nodejs/node#13403 (comment). The main blocker was the missing performance of isPrototypeOf, since it was still a JS builtin backed by a runtime call. This CL also adds more test coverage for the Object.prototype.isPrototypeOf builtin, especially when called from optimized code. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng BUG=v8:5269,v8:5989,v8:6483 [email protected] Review-Url: https://codereview.chromium.org/2934893002 Cr-Commit-Position: refs/heads/master@{#45925}

MylesBorins · 2017-07-17T18:18:05Z

I believe this optimization is specific to a more modern V8. Please let me know if this should be backported

/cc @nodejs/streams @nodejs/v8

bmeurer · 2017-07-17T18:19:53Z

No need to backport.

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jun 2, 2017

mcollina approved these changes Jun 2, 2017

View reviewed changes

addaleax approved these changes Jun 2, 2017

View reviewed changes

bmeurer force-pushed the TurboFan_InstanceOf branch from daf1402 to 6f03b56 Compare June 2, 2017 15:02

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

calvinmetcalf approved these changes Jun 2, 2017

View reviewed changes

cjihrig approved these changes Jun 2, 2017

View reviewed changes

addaleax closed this Jun 9, 2017

addaleax mentioned this pull request Jun 10, 2017

v8.1.1 proposal #13598

Merged

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

BridgeAR mentioned this pull request Aug 25, 2017

lib: var to const #13756

Closed

2 tasks

aliscco mentioned this pull request Apr 27, 2023

[Snyk] Fix for 30 vulnerabilities aliscco/alisco-node#279

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

streams: Ensure that instanceof fast-path is hit. #13403

streams: Ensure that instanceof fast-path is hit. #13403

bmeurer commented Jun 2, 2017 •

edited

Loading

mcollina left a comment

mcollina commented Jun 2, 2017

mscdex commented Jun 2, 2017

jasnell commented Jun 2, 2017

mscdex commented Jun 2, 2017 •

edited

Loading

silverwind commented Jun 2, 2017 •

edited

Loading

mscdex commented Jun 2, 2017 •

edited

Loading

bmeurer commented Jun 2, 2017

bmeurer commented Jun 2, 2017

addaleax commented Jun 9, 2017

MylesBorins commented Jul 17, 2017

bmeurer commented Jul 17, 2017

streams: Ensure that instanceof fast-path is hit. #13403

streams: Ensure that instanceof fast-path is hit. #13403

Conversation

bmeurer commented Jun 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jun 2, 2017

mscdex commented Jun 2, 2017

jasnell commented Jun 2, 2017

mscdex commented Jun 2, 2017 • edited Loading

silverwind commented Jun 2, 2017 • edited Loading

mscdex commented Jun 2, 2017 • edited Loading

bmeurer commented Jun 2, 2017

bmeurer commented Jun 2, 2017

addaleax commented Jun 9, 2017

MylesBorins commented Jul 17, 2017

bmeurer commented Jul 17, 2017

bmeurer commented Jun 2, 2017 •

edited

Loading

mscdex commented Jun 2, 2017 •

edited

Loading

silverwind commented Jun 2, 2017 •

edited

Loading

mscdex commented Jun 2, 2017 •

edited

Loading