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

Transferrable AbortController is very slow #43160

Closed
ronag opened this issue May 20, 2022 · 11 comments
Closed

Transferrable AbortController is very slow #43160

ronag opened this issue May 20, 2022 · 11 comments
Labels
abortcontroller Issues and PRs related to the AbortController API

Comments

@ronag
Copy link
Member

ronag commented May 20, 2022

The way AbortController is implemented (with transferrable support) makes it very slow to use and I'm currently recommending everyone to use the npm package (abort-controller) instead.

I don't think transferrable use of abort controller is very common and it's a bit unfortunate that such an unusual use case has significant performance impact on the common use.

Is there any way to improve this? As far as I understand it's not possible to improve the current implementation with transferrable support without further features from V8. Which leaves the question on whether we can add an option to disable/enable the transferrable implementation? That way users that don't need it can opt-out and get better performance.

@ronag
Copy link
Member Author

ronag commented May 20, 2022

@ronag
Copy link
Member Author

ronag commented May 20, 2022

Even better if we could somehow detect the possible use of transferrable and then dynamically enable it.

@mcollina
Copy link
Member

A quick search on GitHub showed that AbortController is not transferable by the spec: whatwg/dom#438.

I propose we:

  1. make AbortController non-transferable
  2. create a TransferableAbortController with some utility to convert a normal AbortController to it.

@ronag
Copy link
Member Author

ronag commented May 21, 2022

I think then we can implement AbortController in pure js? @benjamingr

@benjamingr
Copy link
Member

The one who pushed for this was @jasnell so I'd want him to weigh in but sgtm.

@F3n67u F3n67u added the abortcontroller Issues and PRs related to the AbortController API label Jun 11, 2022
@jasnell
Copy link
Member

jasnell commented Jun 11, 2022

Sorry for being a bit disconnected. We discussed this issue at the Collab Summit in Austin. We can remove the ability to transfer the AbortController. It is not part of the standard so there's good justification there. It was added as a convenience but given the performance hit, it's fine to remove that.

We don't need a separate TransferableAbortController class. We could, however, have a util.transferableAbortController() or something that returns a regular AbortController that has been marked as transferable per the current behavior. That would give current users a good transition. (An API like AbortController.transferable() would be better but we really shouldn't be hanging non-standard things off AbortController.

That said, I do plan on opening an issue in the DOM spec about making AbortController transferable officially, I just haven't been able to get to it yet.

@mcollina
Copy link
Member

Let's recap the plan:

  1. implementat AbortController in pure JS
  2. add a util.transferableAbortController() in C++

@mcollina
Copy link
Member

Have I missed anything @jasnell?

@jasnell
Copy link
Member

jasnell commented Jun 11, 2022

There's really no reason to implement util.transferableAbortController in c++. Just use the makeTrsnsferable util with a regular AbortController. You can even keep the existing kClone/kDeserialize methods on the existing AbortController class.

@jasnell
Copy link
Member

jasnell commented Jun 12, 2022

PR opened #43388

@mcollina
Copy link
Member

fixed in #44048

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants