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

AbortSignal pattern is slow #44

Closed
ronag opened this issue Jan 20, 2023 · 13 comments
Closed

AbortSignal pattern is slow #44

ronag opened this issue Jan 20, 2023 · 13 comments

Comments

@ronag
Copy link
Member

ronag commented Jan 20, 2023

To the point that e.g. RxJS has decided not support it.

ReactiveX/rxjs#6675 (comment)

@anonrig
Copy link
Member

anonrig commented Feb 9, 2023

What can we do in this area? Any suggestions? @jasnell @mcollina

@jasnell
Copy link
Member

jasnell commented Feb 9, 2023

this was recently addressed. The change may not have been fully backported tho? Not sure.

@anonrig
Copy link
Member

anonrig commented Feb 9, 2023

this was recently addressed. The change may not have been fully backported tho? Not sure.

Which particular pull request are you referring to?

@jasnell
Copy link
Member

jasnell commented Feb 9, 2023

#44048 looks like it was backported to 18.x in #44941 tho

@ronag
Copy link
Member Author

ronag commented Feb 10, 2023

@jasnell do you mean the transferable fix? I think abort signals are still slow but now it's because of eventtarget.

@ronag
Copy link
Member Author

ronag commented Feb 10, 2023

ReactiveX/rxjs#6675 (comment)

After using AbortSignal for a large project that was dealing with high-speed emissions, the performance of AbortSignal, in particular adding and removing event listeners for abort events, was so incredibly poor that I'm unwilling to introduce any flavor of it in RxJS.

We were waiting and waiting for this and thinking maybe we'd break ground on it in 8.x. And now I can't say that I think it's a good idea.

Use takeUntil(fromEvent(signal, 'abort')) in the meantime.

There has a been a lot of focus here on creating signals and events as well as emitting events but that's not actually the problem for the RxJS case...

@ronag
Copy link
Member Author

ronag commented Feb 10, 2023

@benjamingr

@ronag ronag changed the title AbortSignal is slow AbortSignal pattern is slow Feb 10, 2023
@jasnell
Copy link
Member

jasnell commented Feb 10, 2023

Interesting ok. Well, I guess we need to figure out how to speed up EventTarget now. That said, really none of the Web Platform standard apis were designed with performance as a high priority. We'll have to keep find ways of optimizing while still remaining compliant to the specs.

@santigimeno
Copy link
Member

@jasnell any thoughts on this and specially on the Event.isTarget property? Thanks!

@benjamingr
Copy link
Member

@benjamingr

@ronag I'm reading this, I think EventTarget can be made faster (discussion in that thread + a few other ideas).

As for RxJS's case, we can also fast-path util.aborted now that we have that. Basically when a user calls it we don't go through the addEventListener machinery and go through parallel machinery avoiding it.

@anonrig
Copy link
Member

anonrig commented Feb 20, 2023

Referencing: nodejs/node#46648

@benjamingr
Copy link
Member

So - just to elaborate a little on RxJS's case. Us fixing EventTarget wouldn't help them at all since it would still be slow in the browser which they also support.

Their avoidance isn't because our implementation is slow.

@anonrig
Copy link
Member

anonrig commented Apr 3, 2023

Closing since it's a duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants