lib: reuse default DOMException in AbortController by debadree25 · Pull Request #47908 · nodejs/node

debadree25 · 2023-05-07T18:27:19Z

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

Refs: nodejs#46086 Refs: nodejs/performance#44

anonrig · 2023-05-07T18:35:24Z

lib/internal/abort_controller.js

+    if (reason === undefined) {
+      reason = lazyDOMException();
+    }


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

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

No, it won't interfere with it. 👍

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

Oh ok yes reverting

anonrig · 2023-05-07T18:38:05Z

lib/internal/abort_controller.js

-  abort(reason = new DOMException('This operation was aborted', 'AbortError')) {
+  abort(reason) {
+    if (reason === undefined) {
+      reason = lazyDOMException();


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?

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 · 2023-05-07T18:40:35Z

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

aduh95 · 2023-05-07T20:23:52Z

lib/internal/abort_controller.js

-  static abort(
-    reason = new DOMException('This operation was aborted', 'AbortError')) {
+  static abort(reason) {
+    reason ??= lazyDOMException();


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

.length property? didn't get you

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

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

benjamingr

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 · 2023-05-08T04:44:25Z

(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 · 2023-05-08T04:48:35Z

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

benjamingr · 2023-05-08T05:18:41Z

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 · 2023-05-08T05:20:11Z

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 · 2023-05-08T05:43:14Z

IIUC it is for reason only

debadree25 · 2023-05-08T05:59:56Z

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 · 2023-05-08T06:17:19Z

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

debadree25 · 2023-05-08T06:25:46Z

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

debadree25 · 2023-05-08T06:30:01Z

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

lib: reuse default DOMException in AbortController

832efd8

Refs: nodejs#46086 Refs: nodejs/performance#44

nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 7, 2023

debadree25 requested review from anonrig, benjamingr and ronag May 7, 2023 18:28

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

anonrig reviewed May 7, 2023

View reviewed changes

aduh95 reviewed May 7, 2023

View reviewed changes

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

benjamingr requested changes May 8, 2023

View reviewed changes

fixup! use stack trace

5259cbe

debadree25 closed this May 8, 2023

Uh oh!

Conversation

debadree25 commented May 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

debadree25 May 7, 2023 • 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!

anonrig commented May 7, 2023

Uh oh!

aduh95 May 7, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

debadree25 commented May 8, 2023

Uh oh!

debadree25 commented May 8, 2023

Uh oh!

benjamingr commented May 8, 2023

Uh oh!

benjamingr commented May 8, 2023

Uh oh!

debadree25 commented May 8, 2023

Uh oh!

debadree25 commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr commented May 8, 2023

Uh oh!

debadree25 commented May 8, 2023

Uh oh!

debadree25 commented May 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

debadree25 May 7, 2023 •

edited

Loading

aduh95 May 7, 2023 •

edited

Loading

debadree25 commented May 8, 2023 •

edited

Loading