-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
events: improve emitter performance #185889
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: privates use _
in this file, no use of public
unless needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice stuff 👏
Maybe add a test that ensures adjusting the delivery "queue" works when compactation occurs during delivery. This is and the whole private delivery mechanism is definitly under-tested
Out of curiosity: what kind of machine has been used? Can you check the dev tools log for a message like this: "INFO [perf] Render performance baseline is 18ms". Would allow us to guess the real world impact |
that's tested by the "dispose during emission" test 🙂
This is on my desktop PC (Windows, nvidia 3080, amd ryzen 5900X) which has a renderer baseline around ~25ms. |
Looked through the tests and ran event.test.ts with coverage. It looks like we have good coverage with the existing suite; we have complete line and branch coverage except for the leakageMon and unreachable branches. I added a couple more tests, but if you see any angles I missed please let me know! |
ff41645
to
3f2e273
Compare
3f2e273
to
76e9ed7
Compare
This saves about 10ms off our startup time with high probability (p=0.0002).
Closes #185789