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

events: improve emitter performance #185889

Merged
merged 6 commits into from Jun 22, 2023
Merged

events: improve emitter performance #185889

merged 6 commits into from Jun 22, 2023

Conversation

connor4312
Copy link
Member

@connor4312 connor4312 commented Jun 22, 2023

This saves about 10ms off our startup time with high probability (p=0.0002).

Closes #185789

image

@VSCodeTriageBot VSCodeTriageBot added this to the June 2023 milestone Jun 22, 2023
@jrieken jrieken self-requested a review June 22, 2023 16:16
joyceerhl
joyceerhl previously approved these changes Jun 22, 2023
Copy link
Member

@jrieken jrieken left a 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

@jrieken jrieken self-requested a review June 22, 2023 16:45
jrieken
jrieken previously approved these changes Jun 22, 2023
Copy link
Member

@jrieken jrieken left a 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

@jrieken
Copy link
Member

jrieken commented Jun 22, 2023

This saves about 10ms off our startup time with high probability

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

@connor4312
Copy link
Member Author

connor4312 commented Jun 22, 2023

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

that's tested by the "dispose during emission" test 🙂

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

This is on my desktop PC (Windows, nvidia 3080, amd ryzen 5900X) which has a renderer baseline around ~25ms.

@connor4312
Copy link
Member Author

connor4312 commented Jun 22, 2023

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!

bpasero
bpasero previously approved these changes Jun 22, 2023
@connor4312 connor4312 disabled auto-merge June 22, 2023 18:17
bpasero
bpasero previously approved these changes Jun 22, 2023
@connor4312 connor4312 merged commit 6f748a1 into main Jun 22, 2023
6 checks passed
@connor4312 connor4312 deleted the connor4312/faster-events branch June 22, 2023 19:26
@github-actions github-actions bot locked and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VS Code event emitter performance improvements
5 participants