Optimize GreenNode.ToString to not potentially not allocate when it has only a single non-null slot value.#12227
Merged
ToddGrun merged 5 commits intodotnet:mainfrom Sep 15, 2025
Conversation
…as only a single non-null slot value. I see a lot of calls to this where the only non-null slot value is a token, which has a non-allocating ToString implementation.
davidwengier
approved these changes
Sep 15, 2025
Member
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
Makes sense to me. Probably wouldn't achieve any meaningful difference, but the thought did occur to me that for the most common candidates, would it make sense to just override ToString in the syntax class, and directly check the uncommonly filled in slots rather than looping, but its only a for loop so yeah.
ToddGrun
force-pushed
the
dev/toddgrun/Optimize_GreenNode_ToString
branch
from
d5df7be to
8da68b3
Compare
Author
|
I can be convinced otherwise, but it seems to me it makes more sense to handle this generically, rather than needing each class to code their own ToString optimization. From what I could gather from the perf runs, GetSingleSlotValueOrDefault didn't seem to add much in the way of CPU costs. In reply to: 3219295301 |
DustinCampbell
approved these changes
Sep 15, 2025
DustinCampbell
approved these changes
Sep 15, 2025
chsienki
approved these changes
Sep 15, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I see a lot of calls to this where the only non-null slot value is a token, which has a non-allocating ToString implementation.
The numbers from the test insertion were a little hard to decipher. The cohosting test has huge variances in general (which we should try to fix). Of the runs that didn't have the extremely high numbers, it looked like this change reduced allocations under GreenNode.ToString by about 50% while not adding any CPU cost (again, hard to read, but GetSingleSlotValueOrDefault looked like it was an insignificant CPU cost wrt the rest of the ToString impelementation).
Test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/670195