assert: improve error check #17574

BridgeAR · 2017-12-09T16:31:45Z

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

apapirovski

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.

TimothyGu · 2017-12-09T18:33:44Z

lib/internal/errors.js

@@ -140,9 +140,9 @@ class AssertionError extends Error {
    if (message) {
      super(message);
    } else {
-      if (actual && actual.stack && actual instanceof Error)


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.

lpinca · 2017-12-11T15:10:12Z

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

apapirovski · 2017-12-12T17:14:57Z

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/

apapirovski · 2017-12-14T18:45:22Z

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

lpinca · 2017-12-14T19:00:38Z

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

apapirovski · 2017-12-14T19:01:59Z

@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.

lpinca · 2017-12-14T19:58:22Z

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?

apapirovski · 2017-12-14T20:00:35Z

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.

lpinca · 2017-12-14T20:07:03Z

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.

BridgeAR · 2017-12-16T08:00:05Z

@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.

BridgeAR · 2017-12-19T02:36:02Z

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

lpinca · 2017-12-19T07:12:54Z

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

lpinca · 2017-12-19T14:01:02Z

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

BridgeAR · 2017-12-26T09:55:31Z

@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.

BridgeAR · 2018-02-17T00:45:26Z

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.

BridgeAR · 2018-02-17T00:45:34Z

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

BridgeAR · 2018-02-17T00:46:44Z

The commit message should be changed before this lands.

BridgeAR · 2018-02-28T14:58:21Z

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

Minor performance improvement.

BridgeAR · 2018-03-02T19:02:41Z

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

Minor performance improvement. PR-URL: nodejs#17574 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

BridgeAR · 2018-03-06T23:22:11Z

Landed in b5825e1

Minor performance improvement. PR-URL: #17574 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Minor performance improvement. PR-URL: nodejs#17574 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

jasnell · 2018-08-17T23:15:53Z

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 pending requests for changes, 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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: improve error check #17574

assert: improve error check #17574

BridgeAR commented Dec 9, 2017

apapirovski left a comment

TimothyGu Dec 9, 2017

apapirovski Dec 9, 2017 •

edited

Loading

BridgeAR Dec 9, 2017

BridgeAR Dec 11, 2017

TimothyGu Dec 11, 2017

lpinca commented Dec 11, 2017

apapirovski commented Dec 12, 2017

apapirovski commented Dec 14, 2017 •

edited

Loading

lpinca commented Dec 14, 2017

apapirovski commented Dec 14, 2017 •

edited

Loading

lpinca commented Dec 14, 2017

apapirovski commented Dec 14, 2017

lpinca commented Dec 14, 2017 •

edited

Loading

BridgeAR commented Dec 16, 2017

BridgeAR commented Dec 19, 2017

lpinca commented Dec 19, 2017

lpinca commented Dec 19, 2017 •

edited

Loading

BridgeAR commented Dec 26, 2017

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 28, 2018

BridgeAR commented Mar 2, 2018

BridgeAR commented Mar 6, 2018

jasnell commented Aug 17, 2018

assert: improve error check #17574

assert: improve error check #17574

Conversation

BridgeAR commented Dec 9, 2017

Checklist

Affected core subsystem(s)

apapirovski left a comment

Choose a reason for hiding this comment

TimothyGu Dec 9, 2017

Choose a reason for hiding this comment

apapirovski Dec 9, 2017 • edited Loading

Choose a reason for hiding this comment

BridgeAR Dec 9, 2017

Choose a reason for hiding this comment

BridgeAR Dec 11, 2017

Choose a reason for hiding this comment

TimothyGu Dec 11, 2017

Choose a reason for hiding this comment

lpinca commented Dec 11, 2017

apapirovski commented Dec 12, 2017

apapirovski commented Dec 14, 2017 • edited Loading

lpinca commented Dec 14, 2017

apapirovski commented Dec 14, 2017 • edited Loading

lpinca commented Dec 14, 2017

apapirovski commented Dec 14, 2017

lpinca commented Dec 14, 2017 • edited Loading

BridgeAR commented Dec 16, 2017

BridgeAR commented Dec 19, 2017

lpinca commented Dec 19, 2017

lpinca commented Dec 19, 2017 • edited Loading

BridgeAR commented Dec 26, 2017

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 17, 2018

BridgeAR commented Feb 28, 2018

BridgeAR commented Mar 2, 2018

BridgeAR commented Mar 6, 2018

jasnell commented Aug 17, 2018

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