Cache SubstitutedNamedTypeSymbol.GetMembers to avoid unnecessary allocations #69164
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
ghost
added
the
Community
Youssef1313
force-pushed
the
perf-getmembers
branch
from
04c10df
to
2b8d9b5
Compare
Youssef1313
force-pushed
the
perf-getmembers
branch
from
2b8d9b5
to
f24d6b1
Compare
| if (!_lazyMembers.IsDefault) | ||
| { | ||
| // If we already calculated the ordered members, return it directly. | ||
| return _lazyMembers; |
There was a problem hiding this comment.
Yup makes sense. Thanks!
|
Done with review pass (commit 1) |
…embersUnordered`
| if (!_lazyMembers.IsDefault) | ||
| { | ||
| // If we already calculated the ordered members, return it directly. | ||
| return _lazyMembers.ConditionallyDeOrder(); |
There was a problem hiding this comment.
I realize the comment for GetMembersUnordered() states the order may change from call to call, but this may be the first implementation where that is the case (the order will be different before and after _lazyMembers is populated). Is that a concern?
There was a problem hiding this comment.
Is that a concern?
We do not expose GetMembersUnordered to external consumers. However, there is still a chance that an internal implementation somewhere took a dependency on getting items in the same order each time. Reviewing existing call sites might help. What do you think?
There was a problem hiding this comment.
Reviewing existing call sites might help.
There are 50 callsites for NamespaceOrTypeSymbol.GetMembersUnordered(), and a few of those are methods that return that value so the number of use-sites may be too many to review easily. That said, I skimmed through the direct callers of GetMembersUnordered() and didn’t see any obvious issues.
There was a problem hiding this comment.
@Youssef1313, it looks like the perf issue is allocations from GetMembers() and not GetMembersUnordered(). Given that, to avoid the risk of breaking callers of GetMembersUnordered(), let's revert the change to GetMembersUnordered(), and always recalculate that array rather than using _lazyMembers if available. Thanks.
|
Done with review pass (commit 2) |
jcouv
removed
the
untriaged
|
@cston Sorry for leaving this one for long. I addressed your comment. Is this ready to merge now? |
|
Thanks @Youssef1313. |
Fixes #69163