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

Reduce closure and Lazy allocations inside CSharpCompilation.ctor #72371

Merged
merged 7 commits into from
Mar 7, 2024
Merged

Reduce closure and Lazy allocations inside CSharpCompilation.ctor #72371

merged 7 commits into from
Mar 7, 2024

Conversation

ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Mar 2, 2024 •
edited
Loading

Additionally, lazily create a couple members that don't appear to be used on most instances. Together, these account for about 0.8% of all allocations in the CopyPlainText speedometer test.

*** Previous allocations from test profile
image

@ToddGrun
Reduce closure and Lazy allocations inside CSharpCompilation.ctor
89ac45d 
These account for about 0.3% of all allocations in the CopyPlainText speedometer test.
@ToddGrun ToddGrun requested a review from a team as a code owner March 2, 2024 23:11
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 2, 2024
@ToddGrun
add static keyword for clarity
8da54e8 
Reorder bool assignment and CompareExchange to avoid potential usage of member when not initialized
@ToddGrun
Copy link
Contributor Author

ToddGrun commented Mar 5, 2024

@dotnet/roslyn-compiler -- ptal

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 4)

@ToddGrun
Maintain original formatting
0836e9b 
Use lazy naming convention
Move away from StrongBox usage
@ToddGrun
Copy link
Contributor Author

ToddGrun commented Mar 6, 2024

@AlekseyTs any remaining concerns?

AlekseyTs
AlekseyTs reviewed Mar 6, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs Outdated
@@ -92,11 +93,11 @@ internal Conversions Conversions
/// <summary>
/// Manages anonymous types declared in this compilation. Unifies types that are structurally equivalent.
/// </summary>
private readonly AnonymousTypeManager _anonymousTypeManager;
private AnonymousTypeManager? _anonymousTypeManager;
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

_anonymousTypeManager

lazy? #Closed

AlekseyTs
AlekseyTs reviewed Mar 6, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs Outdated

private NamespaceSymbol? _lazyGlobalNamespace;

internal readonly BuiltInOperators builtInOperators;
private BuiltInOperators? _builtInOperators;
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

_builtInOperators

lazy? #Closed

AlekseyTs
AlekseyTs reviewed Mar 6, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs Outdated
@@ -152,7 +153,7 @@ internal Conversions Conversions
/// <summary>
/// Cache of T to Nullable&lt;T&gt;.
/// </summary>
private readonly ConcurrentCache<TypeSymbol, NamedTypeSymbol> _typeToNullableVersion = new ConcurrentCache<TypeSymbol, NamedTypeSymbol>(size: 100);
private ConcurrentCache<TypeSymbol, NamedTypeSymbol>? _typeToNullableVersion;
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

_typeToNullableVersion

lazy? #Closed

AlekseyTs
AlekseyTs reviewed Mar 6, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs Outdated
_options = options;
_lazyScriptClass = ErrorTypeSymbol.UnknownResultType;
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

= ErrorTypeSymbol.UnknownResultType

Consider moving this to a field initializer #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 5)

AlekseyTs
AlekseyTs approved these changes Mar 6, 2024
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)

@build-analysis build-analysis bot mentioned this pull request Mar 6, 2024
Open
3 tasks
@ToddGrun ToddGrun merged commit 5839507 into dotnet:main Mar 7, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharwell sharwell sharwell left review comments

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@RikkiGibson RikkiGibson RikkiGibson 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
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ToddGrun @AlekseyTs @sharwell @RikkiGibson