-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Make GreenNode.CreateList static #48536
Make GreenNode.CreateList static #48536
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
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.
Done review pass (commit 1)
@dotnet/roslyn-compiler can we get a second pair of eyes on this nice community PR? :) |
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.
LGTM (commit 4). @dotnet/roslyn-compiler for a second review. There are no perf numbers in this PR, but Kai is planning on submitting a second PR that actually refactors the implementation of GreenNode.CreateList
that removes a bunch of double-allocations that should have a big impact, and this first change will simplify that.
True. Though changing these virtual methods (and extra parameters and whatnot) to just static calls, should be pure benefit. At worse, we should have no perf change. A best, potentially some uptick :) |
Thanks @HurricanKai! |
As discussed with @CyrusNajmabadi on discord this PR makes GreenNode.CreateList static and adjusts all usages.
This done to simplify the method and remove the virtual call which should help performance somewhat, but I couldn't get perf numbers that could directly confirm this.
I'm not sure whether members should also be reordered. I left them as is for now.