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

perf(web): optimize AbortController #12165

Merged
merged 2 commits into from Sep 21, 2021
Merged

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Sep 21, 2021

  • Use regular class constructor and symbol "private" attributes
  • Lazy init Set of follower signals

Some benchmark data on class init:

class_member_private:    n = 1000000, dt = 0.076s, r = 13157895/s, t = 75ns/op
class_member:            n = 1000000, dt = 0.091s, r = 10989011/s, t = 91ns/op
class_constructor:       n = 1000000, dt = 0.008s, r = 125000000/s, t = 8ns/op
class_symbol_x:          n = 1000000, dt = 0.006s, r = 166666667/s, t = 6ns/op
class_symbol_anon:       n = 1000000, dt = 0.007s, r = 142857143/s, t = 7ns/op

- Use regular class constructor and symbol "private" attributes
- Lazy init set of followee signals
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@AaronO AaronO merged commit f827b93 into denoland:main Sep 21, 2021
@AaronO AaronO deleted the perf/abort-signal branch September 21, 2021 15:39

const illegalConstructorKey = Symbol("illegalConstructorKey");

class AbortSignal extends EventTarget {
#aborted = false;
#abortAlgorithms = new Set();
Copy link
Member

Choose a reason for hiding this comment

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

In the future, it would be good to add comments about performance optimizations so someone doesn't accidentally do something to cause a regression. For example, stating here not to use private properties on this class.

Copy link
Member

Choose a reason for hiding this comment

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

This change will probably have a side effect of leaking these fields during object inspection - AFAIK private fields do not show up during inspection. I'm not sure though if it is against the spec or anything.

Copy link
Member

Choose a reason for hiding this comment

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

For example, stating here not to use private properties on this class.

Well for Web APIs we don't use private properties anywhere, but only symbols. so this was just old code that wasn't updated yet to how we do things.

caspervonb pushed a commit to caspervonb/deno that referenced this pull request Sep 26, 2021
- Use regular class constructor and symbol "private" attributes
- Lazy init Set of follower signals
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants