assert: improve error check by BridgeAR · Pull Request #17574 · nodejs/node · GitHub

With newer V8 versions instanceof checks became very fast and
we do not have to check for the property existence anymore.

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)

assert

Maybe add labels re: not landing on v6.x/v8.x as appropriate. Maybe even v9.x depending on when the optimizations in V8 occurred.

This wasn't a performance microoptimization, but rather an integrity check to avoid Object.create(Error.prototype) from being recognized as an Error.

It was definitely a micro-optimization, see #15025 (and @BridgeAR introduced it). It's possible it has other side-effects although I'm not convinced Object.create(Error.prototype) is something we should worry about in this particular code.

I indeed did not think about this case. I also do not mind if it stays as it will not hurt.

@TimothyGu would you like to keep it to be on the safe side about this or shall I just remove the part again?

@apapirovski Thanks for digging up the original PR. It was very informative. @BridgeAR Feel free to remove.

On Node.js 9.2.1 istanceof is still slow, when was it optimized?

This shouldn't land on v9.x unless V8 6.3 makes its way there.

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

Hmm, does anyone have a relevant benchmark for this? Doing some quick testing in repl indicates this might potentially be OK even on 8.x... cc @lpinca @BridgeAR

@apapirovski I compared instanceof Error and err.stack using https://benchmarkjs.com/ before writing that comment above and it was a lot slower.

@lpinca Right, that makes sense. I assume @BridgeAR was talking about instanceof not being prohibitively slow anymore rather than faster or as fast than the check above. It seems comparable to typeof now whereas it used to be much slower.

If this lands, it can probably land on both v8.x & v9.x because there doesn't seem to be a perf difference.

I didn't test it but I assume current code is still faster when an error is passed. @apapirovski do you have some numbers at hand?

I didn't test it but I assume current code is still faster when an error is passed. @apapirovski do you have some numbers at hand?

I would guess that it definitely is. The question is (probably) whether we want to keep around this type of code or clean it up. I would guess that instanceof is not going to be a bottleneck here.

It makes sense. Reading the discussion in https://github.com/nodejs/node/pull/15025/files it shouldn't probably have been added in the first place.

@apapirovski
Right, that makes sense. I assume @BridgeAR was talking about instanceof not being prohibitively slow anymore rather than faster or as fast than the check above. It seems comparable to typeof now whereas it used to be much slower.

That is correct. instanceof got relatively fast in general since Turbofan landed. I do not remember exactly when. I just stumbled upon this part and did not like it very much. After running some benchmarks I actually decided to take a different way though and optimize it properly. The new code should be faster in all cases and it is also faster in the general case compared to instanceof.

Ping @lpinca @apapirovski @TimothyGu @jasnell @maclover7 what do you think?

Seems good to me, but the in operator wasn't very fast, not sure if it is now.

FWIW according to https://v8project.blogspot.it/2017/12/v8-release-64.html the instanceof operator is 3.6 times faster on V8 6.4

@lpinca that was the change that reminded me about it (https://chromium.googlesource.com/v8/v8.git/+/bcee140617fe90e8c354394216225615b9558113). I am going to check if this specific case is actually better with instanceof after 6.4 landed or if in is still faster. When I ran my benchmarks the current implementation was still better than other options.

So I just checked this again and the current solution is still faster on master. It is also faster on v.8 and v.9 but not on v.6.

The performance divers depending on the input. But this is always faster than the current implementation.

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

The commit message should be changed before this lands.

Ping @lpinca @apapirovski @jasnell @TimothyGu @maclover7 PTAL

Minor performance improvement.

CI before landing https://ci.nodejs.org/job/node-test-pull-request/13455/

Minor performance improvement. PR-URL: nodejs#17574 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Landed in b5825e1

Minor performance improvement. PR-URL: #17574 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Minor performance improvement. PR-URL: nodejs#17574 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Should this be backported to 8.x? If so, we need a separate backport PR.

nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Dec 9, 2017

apapirovski approved these changes Dec 9, 2017

View reviewed changes

BridgeAR added dont-land-on-v8.x labels Dec 9, 2017

maclover7 approved these changes Dec 9, 2017

View reviewed changes

TimothyGu reviewed Dec 9, 2017

View reviewed changes

apapirovski added the dont-land-on-v9.x label Dec 12, 2017

TimothyGu approved these changes Dec 12, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

apapirovski removed dont-land-on-v8.x labels Dec 14, 2017

lpinca approved these changes Dec 14, 2017

View reviewed changes

BridgeAR added blocked PRs that are blocked by other issues or PRs. dont-land-on-v8.x labels Dec 26, 2017

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR force-pushed the fast-instanceof branch from c61bb73 to c4b51c1 Compare February 17, 2018 00:31

BridgeAR added dont-land-on-v6.x and removed blocked PRs that are blocked by other issues or PRs. dont-land-on-v6.x labels Feb 17, 2018

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 17, 2018

BridgeAR changed the title assert: remove obsolete micro-optimization assert: improve error check Feb 17, 2018

lpinca approved these changes Feb 28, 2018

View reviewed changes

assert: improve error check

cc1eae9

Minor performance improvement.

BridgeAR force-pushed the fast-instanceof branch from c4b51c1 to cc1eae9 Compare March 2, 2018 19:01

BridgeAR closed this Mar 6, 2018

targos removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 17, 2018

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

jasnell added the backport-requested-v8.x label Aug 17, 2018

BridgeAR deleted the fast-instanceof branch April 1, 2019 23:39

assert: improve error check by BridgeAR · Pull Request #17574 · nodejs/node · GitHub

Uh oh!

Uh oh!

assert: improve error check - #17574

assert: improve error check#17574BridgeAR wants to merge 1 commit intonodejs:masternodejs/node:masterfrom BridgeAR:fast-instanceofBridgeAR/node:fast-instanceofCopy head branch name to clipboard

Conversation

BridgeAR commented Dec 9, 2017

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Dec 9, 2017

Choose a reason for hiding this comment

Uh oh!

apapirovski Dec 9, 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!

BridgeAR Dec 9, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

lpinca commented Dec 11, 2017

Uh oh!

apapirovski commented Dec 12, 2017

Uh oh!

apapirovski commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Dec 14, 2017

Uh oh!

apapirovski commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Dec 14, 2017

Uh oh!

apapirovski commented Dec 14, 2017

Uh oh!

lpinca commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Dec 16, 2017

Uh oh!

BridgeAR commented Dec 19, 2017

Uh oh!

lpinca commented Dec 19, 2017

Uh oh!

lpinca commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Dec 26, 2017

Uh oh!

BridgeAR commented Feb 17, 2018

Uh oh!

BridgeAR commented Feb 17, 2018

Uh oh!

BridgeAR commented Feb 17, 2018

Uh oh!

BridgeAR commented Feb 28, 2018

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

BridgeAR commented Mar 6, 2018

Uh oh!

jasnell commented Aug 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

assert: improve error check#17574
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:fast-instanceof

apapirovski Dec 9, 2017 •
edited
Loading

apapirovski commented Dec 14, 2017 •
edited
Loading

apapirovski commented Dec 14, 2017 •
edited
Loading

lpinca commented Dec 14, 2017 •
edited
Loading

lpinca commented Dec 19, 2017 •
edited
Loading