-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Switch deferredNodes
from a Map
to a Set
.
#46751
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
Conversation
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@typescript-bot perf test this |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 05124b7. You can monitor the build here. Update: The results are in! |
@DanielRosenwasser Here they are:Comparison Report - main..46751
System
Hosts
Scenarios
Developer Information: |
It certainly makes more sense conceptually, but it's not at all obvious to me that this would result in a performance improvement and, if it does, that that improvement would apply to more than one environment. |
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.
If @rbuckton doesn't have specific reasons for keeping it like this, LGTM
Something I'm extracting from a larger set of experiments in #46595. What I think I've found is that there's a very small improvement from switching
deferredNodes
from aMap
to aSet
, and, well, it makes more sense anyway.