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

lib: reuse default DOMException in AbortController #47908

Closed

Conversation

debadree25
Copy link
Member

Similar to #46086 have reused the default DOMException we throw when calling ac.abort(), so far the tests dont break and the benchmark added in this PR show good improvement

Benchmark results:

                                          confidence improvement accuracy (*)   (**)  (***)
events/abortcontroller-abort.js n=1000000        ***    148.36 %       ±2.47% ±3.30% ±4.33%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 1 comparisons, you can thus expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

Refs: #46086
Refs: nodejs/performance#44

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 7, 2023
@debadree25 debadree25 added performance Issues and PRs related to the performance of Node.js. abortcontroller Issues and PRs related to the AbortController API labels May 7, 2023
Comment on lines 189 to 191
if (reason === undefined) {
reason = lazyDOMException();
}
Copy link
Member

Choose a reason for hiding this comment

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

Would reason ??= lazyDOMException(); also work in here, and in the next usage?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so yes! updating this (since benchmark ci is running would pushing a commit interfere with it?)

Copy link
Member

Choose a reason for hiding this comment

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

No, it won't interfere with it. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t think this is correct, users should be able to supply null as a reason

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh ok yes reverting

abort(reason = new DOMException('This operation was aborted', 'AbortError')) {
abort(reason) {
if (reason === undefined) {
reason = lazyDOMException();
Copy link
Member

Choose a reason for hiding this comment

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

Since we are not creating a new error everytime abort is called, 2 different usages in the code throwing this error, might have a different stack trace. Is this compliant with the spec?

Copy link
Member Author

@debadree25 debadree25 May 7, 2023

Choose a reason for hiding this comment

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

ah good point, not sure about that, one thing we could do maybe is move this lazy loading inside the functions themselves. that should ensure correct trace?

@anonrig
Copy link
Member

anonrig commented May 7, 2023

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

static abort(
reason = new DOMException('This operation was aborted', 'AbortError')) {
static abort(reason) {
reason ??= lazyDOMException();
Copy link
Contributor

@aduh95 aduh95 May 7, 2023

Choose a reason for hiding this comment

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

I think this won’t let users define a null abort reason. Any reason to use this syntax over what was before? Also it changes the .length property of the static method from 0 to 1

Copy link
Member Author

Choose a reason for hiding this comment

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

.length property? didn't get you

Copy link
Member

Choose a reason for hiding this comment

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

The function's .length property (controller.abort.length) which is typically not important but in web standards like AbortController it is

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok got it, it expects the reason param now thats why

@debadree25 debadree25 force-pushed the ft/abort-controller-lazy-exp branch from d2923f0 to 832efd8 Compare May 8, 2023 04:35
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Good work but as others mentioned this means that all the aborts get the same stack trace and it's impossible to figure out where an error came from so the performance benefit is probably not worth the debugging downside of knowing who called ".abort" which is currently possible since the error is captured in .abort but now becomes impossible IIUC.

@debadree25
Copy link
Member Author

(I have no idea just asking) is there maybe a way to recapture the trace and inject the details into the lazy loaded one?

@debadree25
Copy link
Member Author

I think something like Error.captureStackTrace could help? or would it just erase the gains.
Testing it
Any suggestions would be helpful

@benjamingr
Copy link
Member

I'm afraid creating the error and capturing the stack is the expensive part so I'm not sure, it may be worth while to optimize DOMException itself

@benjamingr
Copy link
Member

It may be fast to not create the DOMException in abort() lazily, why do we create it in the first place? Is it just for .reason?

@debadree25
Copy link
Member Author

IIUC it is for reason only

@debadree25
Copy link
Member Author

debadree25 commented May 8, 2023

Hey so in 5259cbe I did the following

  • Set the default value in the static abort function to undefined
  • I updated to use Error.captureStackTrace()
    For testing stack traces i used the following script
'use strict';
const ac1 = new AbortController();
ac1.signal.addEventListener('abort', () => {
  console.log(ac1.signal.reason);
});
const ac2 = new AbortController();
ac2.signal.addEventListener('abort', () => {
  console.log(ac2.signal.reason);
});
function abortThis(controller) {
  controller.abort();
}
function abortThis2(controller) {
  controller.abort();
}
setTimeout(() => {
  abortThis(ac1);
  setTimeout(() => {
    abortThis2(ac2);
  }, 100);
}, 100);

On normal node trace looks like this

DOMException [AbortError]: This operation was aborted
    at new DOMException (node:internal/per_context/domexception:53:5)
    at AbortController.abort (node:internal/abort_controller:331:18)
    at abortThis (/Users/debadreechatterjee/Documents/Personal/node/test.js:11:14)
    at Timeout._onTimeout (/Users/debadreechatterjee/Documents/Personal/node/test.js:17:3)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)
DOMException [AbortError]: This operation was aborted
    at new DOMException (node:internal/per_context/domexception:53:5)
    at AbortController.abort (node:internal/abort_controller:331:18)
    at abortThis2 (/Users/debadreechatterjee/Documents/Personal/node/test.js:14:14)
    at Timeout._onTimeout (/Users/debadreechatterjee/Documents/Personal/node/test.js:19:5)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)

On this PR

DOMException [AbortError]: This operation was aborted
    at AbortController.abort (node:internal/abort_controller:352:7)
    at abortThis (/Users/debadreechatterjee/Documents/Personal/node/test.js:11:14)
    at Timeout._onTimeout (/Users/debadreechatterjee/Documents/Personal/node/test.js:17:3)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)
DOMException [AbortError]: This operation was aborted
    at AbortController.abort (node:internal/abort_controller:352:7)
    at abortThis2 (/Users/debadreechatterjee/Documents/Personal/node/test.js:14:14)
    at Timeout._onTimeout (/Users/debadreechatterjee/Documents/Personal/node/test.js:19:5)
    at listOnTimeout (node:internal/timers:573:17)
    at process.processTimers (node:internal/timers:514:7)

and the performance benefit still seems to exist, although not as impressive as the initial one:

                                          confidence improvement accuracy (*)   (**)  (***)
events/abortcontroller-abort.js n=1000000        ***     20.64 %       ±3.77% ±5.01% ±6.53%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 1 comparisons, you can thus expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

wdyt @benjamingr

@benjamingr
Copy link
Member

Wouldn't that replace the stack trace on earlier references to the error "under the hood" since it's the same reference?

@debadree25
Copy link
Member Author

Ah oh no yes, I am not thinking straight 😓😓 I guess then this a dead end 😕

@debadree25
Copy link
Member Author

Probably no other way, for now, maybe we could optimize DOMException, I am closing this for now, If there are any other ideas I think we can discuss on this PR thread

@debadree25 debadree25 closed this May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abortcontroller Issues and PRs related to the AbortController API 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.

5 participants