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.

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

Use new helper type for initializing a value type field only once, and in a threadsafe manner. #71013

Merged
merged 5 commits into from
Dec 20, 2023

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner November 29, 2023 17:16
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 29, 2023
@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler we noticed these few places that could potentially benefit moving off of StrongBox to the SingleInitNullable type. That type is the same size as Nullable<T>, but ensures it will only be initialized once, and that the initialization is threadsafe (no other threads can read the value until fully written).

It's up to you if you want to take this or not. Feel free to close out if you're happy with the code as is.

@@ -34,6 +34,10 @@ private static bool TryGetTargetScope(SuppressMessageInfo info, out TargetScope
private readonly Compilation _compilation;
private GlobalSuppressions? _lazyGlobalSuppressions;
private readonly ConcurrentDictionary<ISymbol, ImmutableDictionary<string, SuppressMessageInfo>> _localSuppressionsBySymbol;

// These are StrongBoxes because 'null' is a valid symbol value to compute for these, and as such, we can't use
// the null value to indicate 'not yet computed'.
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Nov 29, 2023

Choose a reason for hiding this comment

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

feel free to delete this comment as well. i just thought it might be helpful for someone asking why a reference type needed to be wrapped in a strongbox.

IMO though, this should use Optional<T> But we'd also need a way to initialize that in an interlocked fashion.

@@ -27,7 +27,7 @@ internal class BuiltInOperators
//actual lazily-constructed caches of built-in operators.
private ImmutableArray<UnaryOperatorSignature>[] _builtInUnaryOperators;
private ImmutableArray<BinaryOperatorSignature>[][] _builtInOperators;
private StrongBox<BinaryOperatorSignature> _builtInUtf8Concatenation;
private SingleInitNullable<BinaryOperatorSignature> _builtInUtf8Concatenation;
Copy link
Member Author

Choose a reason for hiding this comment

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

note: this also avoids a heap allocation here.

@@ -754,7 +745,7 @@ bool isGeneratedHeuristic()

private CSharpLineDirectiveMap? _lazyLineDirectiveMap;
private CSharpPragmaWarningStateMap? _lazyPragmaWarningStateMap;
private StrongBox<NullableContextStateMap>? _lazyNullableContextStateMap;
private SingleInitNullable<NullableContextStateMap> _lazyNullableContextStateMap;
Copy link
Member Author

Choose a reason for hiding this comment

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

note: this also avoids a heap allocation here.

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler @333fred @RikkiGibson ptal.

@CyrusNajmabadi
Copy link
Member Author

@jaredpar ptal. :)

@CyrusNajmabadi CyrusNajmabadi merged commit 67ef2fb into dotnet:main Dec 20, 2023
25 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the useSingleInit branch December 20, 2023 16:30
@ghost ghost added this to the Next milestone Dec 20, 2023
@Cosifne Cosifne modified the milestones: Next, 17.9 P3 Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants