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.

Sign up for GitHub

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

Jump to bottom

Pool instances of SymbolDisplayVisitor #70363

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Pool instances of SymbolDisplayVisitor #70363

merged 3 commits into from
Nov 10, 2023

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Oct 12, 2023 •
edited by Pilchie
Loading

Addresses the 3rd most allocated object in AB#1899592 (123MB / 12,7%).

Review commit-by-commit recommended

@sharwell sharwell requested a review from a team as a code owner October 12, 2023 22:57
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 12, 2023
@sharwell sharwell marked this pull request as draft October 13, 2023 02:35
@sharwell sharwell marked this pull request as ready for review October 13, 2023 02:49
AlekseyTs
AlekseyTs reviewed Oct 17, 2023
View reviewed changes
src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs Outdated
@@ -385,15 +384,15 @@ public override void VisitMethod(IMethodSymbol symbol)
{
containingType = symbol.ReceiverType;
includeType = true;
Debug.Assert(containingType != null);
RoslynDebug.Assert(containingType != null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RoslynDebug

Does this change make a difference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➡️ Currently, no. If in the future we have the ability to analyze linked variables (in this case, an effective 'not null when includeType is true'), it would make a difference.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If in the future we have the ability to analyze linked variables (in this case, an effective 'not null when includeType is true'), it would make a difference.

Why that would make a difference for RoslynDebug.Assert, but not for Debug.Assert?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug.Assert is not annotated in all target frameworks, but RoslynDebug.Assert is. In general, null checks should always go through RoslynDebug so the annotation applies consistently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I am buying this. I think Debug.Assert works fine for the purpose of nullable analysis in this project, and I would prefer us transitioning in the opposite direction, i.e. stop using RoslynDebug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➡️ Switched all uses of RoslynDebug in this PR to Debug

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 4)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 5). Overall, LGTM, but there are two remaining questions. I would like to get them resolved before signing off.

AlekseyTs
AlekseyTs approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 6)

@sharwell
Copy link
Member Author

@dotnet/roslyn-compiler for second review

333fred
333fred reviewed Oct 19, 2023
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment. Otherwise looking good.

src/Compilers/Core/Portable/SymbolDisplay/AbstractSymbolDisplayVisitor.cs Outdated
FreeNotFirstVisitor(_lazyNotFirstVisitorNamespaceOrType);

_builder = null!;
_format = null!;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If _format is the field we're using as a sentinel of "this thing is currently in use", we should set it to null last.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➡️ I decided to keep these in the order the fields are declared in the type, so it's easy to see they are all included. The null check is not intended to suggest that the type is thread-safe; it merely serves to document a precondition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The null check is not intended to suggest that the type is thread-safe; it merely serves to document a precondition.

It doesn't suggest the type is threadsafe, but it does suggest that we're taking some amount of care to ensure that SymbolDisplayVisitor isn't being reused by multiple things at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➡️ Relocated the assignment so this can get merged

@sharwell
Copy link
Member Author

After review, rebased to combine commits with no content changes.

@sharwell sharwell merged commit 358d78d into dotnet:main Nov 10, 2023
25 checks passed
@ghost ghost added this to the Next milestone Nov 10, 2023
@allisonchou allisonchou mentioned this pull request Nov 17, 2023
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 17.9 P2 Nov 28, 2023
@sharwell sharwell deleted the pool-visitor branch January 16, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@333fred 333fred 333fred approved these changes

Assignees
No one assigned
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.9 P2
Development

Successfully merging this pull request may close these issues.
5 participants
@sharwell @AlekseyTs @333fred @CyrusNajmabadi @RikkiGibson