Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

errors: improve performance of NodeError instantiation #49654

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Sep 14, 2023

Well. I dont know why captureLargerStackTrace exists.

Maybe my approach is wrong. I dont know if I lack some node specific knowledge.

I tried to close the gap between the v8 Error class and NodeError. If we really need to support the instantiation of Errors without new, we could wrap it in function. But on the other hand, other Errors are also class type.

The tests basically pass locally, except /test/parallel/test-snapshot-worker.js. Maybe I messed something up (hope not :D)

So please dont be too harsh. 🫣

benchmarks:

Main:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ node benchmark/error/node-error.js #
error/node-error.js stackTraceLimit=0 code="built-in" n=1000000: 2,740,941.413385955
error/node-error.js stackTraceLimit=10 code="built-in" n=1000000: 530,888.1376037208
error/node-error.js stackTraceLimit=0 code="ERR_HTTP2_STREAM_SELF_DEPENDENCY" n=1000000: 160,106.7862310844
error/node-error.js stackTraceLimit=10 code="ERR_HTTP2_STREAM_SELF_DEPENDENCY" n=1000000: 158,231.65339727345

PR

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ ./node benchmark/error/node-error.js #
error/node-error.js stackTraceLimit=0 code="built-in" n=1000000: 2,755,590.7105245455
error/node-error.js stackTraceLimit=10 code="built-in" n=1000000: 517,735.4538075898
error/node-error.js stackTraceLimit=0 code="ERR_HTTP2_STREAM_SELF_DEPENDENCY" n=1000000: 2,243,410.1895228666
error/node-error.js stackTraceLimit=10 code="ERR_HTTP2_STREAM_SELF_DEPENDENCY" n=1000000: 413,506.70459812187

So before we have about 170k and with the PR we have 413k. So we gain what? 240%?

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/loaders
  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 14, 2023
@aduh95
Copy link
Contributor

aduh95 commented Sep 14, 2023

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

Results
                                                                                         confidence improvement accuracy (*)    (**)   (***)
error/error.js n=10000000                                                                               -0.66 %       ±1.20%  ±1.59%  ±2.08%
error/hidestackframes.js n=100000 type='direct-call-noerr'                                               2.16 %       ±4.40%  ±5.86%  ±7.62%
error/hidestackframes.js n=100000 type='direct-call-throw'                                      ***    105.53 %       ±3.23%  ±4.31%  ±5.63%
error/hidestackframes.js n=100000 type='hide-stackframes-noerr'                                          1.63 %       ±5.28%  ±7.03%  ±9.17%
error/hidestackframes.js n=100000 type='hide-stackframes-throw'                                 ***    102.13 %       ±4.01%  ±5.36%  ±7.03%
error/node-error.js stackTraceLimit=0 code='built-in' n=1000000                                   *      3.45 %       ±3.24%  ±4.32%  ±5.63%
error/node-error.js stackTraceLimit=0 code='ERR_HTTP2_STREAM_SELF_DEPENDENCY' n=1000000         ***   1505.54 %      ±38.12% ±51.37% ±68.18%
error/node-error.js stackTraceLimit=10 code='built-in' n=1000000                                        -1.31 %       ±1.80%  ±2.40%  ±3.12%
error/node-error.js stackTraceLimit=10 code='ERR_HTTP2_STREAM_SELF_DEPENDENCY' n=1000000        ***    201.56 %       ±3.60%  ±4.81%  ±6.29%

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 14, 2023

Do the tests need to pass to trigger the benchmark?

@rluvaton
Copy link
Member

Do the tests need to pass to trigger the benchmark?

no, it's a separate process

@rluvaton rluvaton added performance Issues and PRs related to the performance of Node.js. needs-benchmark-ci PR that need a benchmark CI run. labels Sep 14, 2023
@Uzlopak

This comment was marked as outdated.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, looks really good

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 15, 2023

I had to patch the wpt testharness, because it checks if the constructor is equal the BaseClass. I tried to figure out if I somehow could cheat the logic. It worked in a user land script but not with the NodeError in NodeJs.
So i dont know if this is a break dealer, or not. I could propose a PR for the wpt testharness repo, but it seems to have alot of PRs unprocessed and if we dont have somebody on the wpr project it wouldnt get merged soon.

I worked 2 days on this PR and I tested a lot of variations on how to get it with prototype inheritance, avoid the ObjectDefineProperties and keep the attributes non-enumarable. But no chance. Only by using the proposed ES6 class I could achieve the desires behaviour.

Regarding the remaining error I really have no clue why it does not create the desired error message. I am really confused. So this why i also proposed the PR, because I a have no clue what the issue is. I thought maybe I messed up hard and somehow share resources between the errors. But no. So if somebody could help with this, because I really dont know what the issue is.

By using an es6 class we use v8s own captureStackTrace Logic and hide the Error with it.

It would also mean, that if we use this solution for all errors of node, that we optimize it basically to the maximum. So for more performance we need then to optimize v8.

@rluvaton
Copy link
Member

Benchmark CI output:

00:54:38.751                                                                                          confidence improvement accuracy (*)    (**)   (***)
00:54:38.751 error/error.js n=10000000                                                                               -0.66 %       ±1.20%  ±1.59%  ±2.08%
00:54:38.751 error/hidestackframes.js n=100000 type='direct-call-noerr'                                               2.16 %       ±4.40%  ±5.86%  ±7.62%
00:54:38.751 error/hidestackframes.js n=100000 type='direct-call-throw'                                      ***    105.53 %       ±3.23%  ±4.31%  ±5.63%
00:54:38.751 error/hidestackframes.js n=100000 type='hide-stackframes-noerr'                                          1.63 %       ±5.28%  ±7.03%  ±9.17%
00:54:38.751 error/hidestackframes.js n=100000 type='hide-stackframes-throw'                                 ***    102.13 %       ±4.01%  ±5.36%  ±7.03%
00:54:38.751 error/node-error.js stackTraceLimit=0 code='built-in' n=1000000                                   *      3.45 %       ±3.24%  ±4.32%  ±5.63%
00:54:38.751 error/node-error.js stackTraceLimit=0 code='ERR_HTTP2_STREAM_SELF_DEPENDENCY' n=1000000         ***   1505.54 %      ±38.12% ±51.37% ±68.18%
00:54:38.751 error/node-error.js stackTraceLimit=10 code='built-in' n=1000000                                        -1.31 %       ±1.80%  ±2.40%  ±3.12%
00:54:38.751 error/node-error.js stackTraceLimit=10 code='ERR_HTTP2_STREAM_SELF_DEPENDENCY' n=1000000        ***    201.56 %       ±3.60%  ±4.81%  ±6.29%
00:54:38.751 
00:54:38.752 Be aware that when doing many comparisons the risk of a false-positive
00:54:38.752 result increases. In this case, there are 9 comparisons, you can thus
00:54:38.752 expect the following amount of false-positive results:
00:54:38.752   0.45 false positives, when considering a   5% risk acceptance (*, **, ***),
00:54:38.752   0.09 false positives, when considering a   1% risk acceptance (**, ***),
00:54:38.752   0.01 false positives, when considering a 0.1% risk acceptance (***)

well done

lib/internal/errors.js Outdated Show resolved Hide resolved
test/fixtures/wpt/resources/testharness.js Outdated Show resolved Hide resolved
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 15, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 15, 2023
@nodejs-github-bot
Copy link
Collaborator

@benjamingr
Copy link
Member

Note this means Node gives worse errors since captureLargerStackTrace was used to add more stack frames in case Error.stackTraceLimit was set to a low value (which likely means the user would get no meaningful frames from outside of core) I think?

I think 656ce92#diff-670bf55805b781d9a3579f8bca9104c04d94af87cc33220149fd7d37b095ca1cR413 is the change that added it #35644

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 16, 2023

@benjamingr

My assessment:
I think the reason why we needed captureLargerStackTrace was because the original NodeError function would instantiate first the (base) Error and add the properties to it and return the Error. So when you called e.g. throw new ERR_INVALID_ARG_TYPE you would always have also the NodeError function call in your stack trace as the first entry, which we dont want to show in the stacktrace as it is some node internal specific information nobody cares of. So we captured all stack traces and removed basically the top call away and applied the limit.

Now that we dont need to instantiate the base error but directly generate the error instance, the NodeError StackFrame is not created anymore. So we can use the v8 stacktrace functionality directly, without the need to manually limit the stacktrace.

The new logic:

'use strict';

const kIsNodeError = Symbol('kIsNodeError');

function makeNodeErrorWithCode(Base, key) {
    class NodeError extends Base {
      #message = '';
      code = key;
  
      constructor(...args) {
        super();
        this.message = 'bla'
      }
      get ['constructor']() {
        return Base;
      }
  
      get [kIsNodeError]() {
        return true;
      }
  
      get message() {
        return this.#message;
      }
  
      set message(value) {
        this.#message = value;
      }
  
      [Symbol.toStringTag] = 'Error'

      toString() {
        return `${this.name} [${key}]: ${this.message}`;
      }
    }
    Object.setPrototypeOf(NodeError.constructor, Base)
    return NodeError;
  }

const base = TypeError

const Err = makeNodeErrorWithCode(base, 'ERR_INVALID_URL');

function test() {
  try {
    test1()
  } catch (e) {
    console.log(e.stack)
  }
}

function test1() {
  test2()
}

function test2() {
  test3()
}

function test3() {
  test4()
}

function test4() {
  test5()
}

function test5() {
  test6()
}

function test6() {
  test7()
}

function test7() {
  test8()
}

function test8() {
  test9()
}

function test9() {
  test10()
}

function test10() {
  throw new Err()
}

test()
aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ ./node test.js 
TypeError: bla
    at test10 (/home/aras/workspace/node/test.js:89:9)
    at test9 (/home/aras/workspace/node/test.js:85:3)
    at test8 (/home/aras/workspace/node/test.js:81:3)
    at test7 (/home/aras/workspace/node/test.js:77:3)
    at test6 (/home/aras/workspace/node/test.js:73:3)
    at test5 (/home/aras/workspace/node/test.js:69:3)
    at test4 (/home/aras/workspace/node/test.js:65:3)
    at test3 (/home/aras/workspace/node/test.js:61:3)
    at test2 (/home/aras/workspace/node/test.js:57:3)
    at test1 (/home/aras/workspace/node/test.js:53:3)

vs. the old logic

'use strict';

const kIsNodeError = Symbol('kIsNodeError');
function makeNodeErrorWithCode(Base, key) {
  return function NodeError(...args) {
    const limit = Error.stackTraceLimit;
    Error.stackTraceLimit = 0;
    const error = new Base();
    // Reset the limit and setting the name property.
    Error.stackTraceLimit = limit;

    const message = 'bla'
    Object.defineProperties(error, {
      [kIsNodeError]: {
        __proto__: null,
        value: true,
        enumerable: false,
        writable: false,
        configurable: true,
      },
      message: {
        __proto__: null,
        value: message,
        enumerable: false,
        writable: true,
        configurable: true,
      },
      toString: {
        __proto__: null,
        value() {
          return `${this.name} [${key}]: ${this.message}`;
        },
        enumerable: false,
        writable: true,
        configurable: true,
      },
    });

    Error.captureStackTrace(error);
    error.code = key;
    return error;
  };
}

const base = TypeError

const Err = makeNodeErrorWithCode(base, 'ERR_INVALID_URL');

function test() {
  try {
    test1()
  } catch (e) {
    console.log(e.stack)
  }
}

function test1() {
  test2()
}

function test2() {
  test3()
}

function test3() {
  test4()
}

function test4() {
  test5()
}

function test5() {
  test6()
}

function test6() {
  test7()
}

function test7() {
  test8()
}

function test8() {
  test9()
}

function test9() {
  test10()
}

function test10() {
  throw new Err()
}

Error.stackTraceLimit = 10

test()
TypeError: bla
    at new NodeError (/home/aras/workspace/node/test.js:39:11)
    at test10 (/home/aras/workspace/node/test.js:94:9)
    at test9 (/home/aras/workspace/node/test.js:90:3)
    at test8 (/home/aras/workspace/node/test.js:86:3)
    at test7 (/home/aras/workspace/node/test.js:82:3)
    at test6 (/home/aras/workspace/node/test.js:78:3)
    at test5 (/home/aras/workspace/node/test.js:74:3)
    at test4 (/home/aras/workspace/node/test.js:70:3)
    at test3 (/home/aras/workspace/node/test.js:66:3)
    at test2 (/home/aras/workspace/node/test.js:62:3)

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 16, 2023

@joyeecheung

Can you please help me, regarding this PR? I cant figure out why ERR_NOT_SUPPORTED_IN_SNAPSHOT is not having the correct message.

lib/internal/errors.js Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 17, 2023

@joyeecheung

Is this better?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 17, 2023

@joyeecheung

Should the other errors also become enumerable message attributes?

ERR_NOT_BUILDING_SNAPSHOT
ERR_DUPLICATE_STARTUP_SNAPSHOT_MAIN_FUNCTION

@Uzlopak Uzlopak changed the title errors: improve performance of instantiation errors: improve performance of NodeError instantiation Sep 18, 2023
@anonrig anonrig added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 28, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 28, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/49654
✔  Done loading data for nodejs/node/pull/49654
----------------------------------- PR info ------------------------------------
Title      errors: improve performance of NodeError instantiation (#49654)
Author     Aras Abbasi  (@Uzlopak)
Branch     Uzlopak:improve-perf-error-creation -> nodejs:main
Labels     lib / src, performance, needs-ci, needs-benchmark-ci, commit-queue-squash
Commits    1
 - errors: improve performance of instantiation
Committers 1
 - uzlopak 
PR-URL: https://github.com/nodejs/node/pull/49654
Reviewed-By: Matteo Collina 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Stephen Belanger 
Reviewed-By: Joyee Cheung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49654
Reviewed-By: Matteo Collina 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Stephen Belanger 
Reviewed-By: Joyee Cheung 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - errors: improve performance of instantiation
   ℹ  This PR was created on Thu, 14 Sep 2023 19:12:31 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49654#pullrequestreview-1628257155
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/49654#pullrequestreview-1642822449
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/49654#pullrequestreview-1633759369
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/49654#pullrequestreview-1640186686
   ✔  Last GitHub CI successful
   ℹ  Last Benchmark CI on 2023-09-25T21:46:15Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1419/
   ℹ  Last Full PR CI on 2023-09-28T09:00:48Z: https://ci.nodejs.org/job/node-test-pull-request/54318/
- Querying data for job/node-test-pull-request/54318/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/6337251698

@rluvaton rluvaton added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 28, 2023
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 28, 2023

Ah come on...

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 28, 2023
@nodejs-github-bot nodejs-github-bot merged commit cc725a6 into nodejs:main Sep 28, 2023
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in cc725a6

@Uzlopak Uzlopak deleted the improve-perf-error-creation branch September 28, 2023 09:58
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 28, 2023

YEAH

@rluvaton
Copy link
Member

well done @Uzlopak !

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 28, 2023

Thank you. Next step is improving perf of SystemError

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49654
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49654
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49654
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants