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 TypeConversions allocations by caching them in AssemblySymbol #69040

Merged
merged 8 commits into from
Aug 16, 2023
Merged

Reduce TypeConversions allocations by caching them in AssemblySymbol #69040

merged 8 commits into from
Aug 16, 2023

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jul 14, 2023 •
edited
Loading

Reduce TypeConversions allocations by caching this to the AssemblySymbol. Fixes #68997

@Youssef1313 Youssef1313 marked this pull request as ready for review July 14, 2023 20:00
@Youssef1313 Youssef1313 requested a review from a team as a code owner July 14, 2023 20:00
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 14, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jul 14, 2023
@Youssef1313 Youssef1313 mentioned this pull request Jul 14, 2023
Closed
@Youssef1313
Copy link
Member Author

@RikkiGibson @333fred @AlekseyTs Could I get a review on this? Thanks!

AlekseyTs
AlekseyTs reviewed Jul 17, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/AssemblySymbol.cs Outdated
{
if (_lazyTypeConversions is null)
{
Interlocked.CompareExchange(ref _lazyTypeConversions, new TypeConversions(this), null);
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

new TypeConversions(this)

It looks like this would be incorrect if this assembly is not a core library. Instead we should delegate to the core library then. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlekseyTs I'm not following with this.

Some existing code used to do new TypeConversions(ContainingAssembly) where ContainingAssembly doesn't seem guaranteed to be corlib.

My theory is that it's the caller responsibility to call the property for the correct assembly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some existing code used to do new TypeConversions(ContainingAssembly) where ContainingAssembly doesn't seem guaranteed to be corlib.

My theory is that it's the caller responsibility to call the property for the correct assembly.

I think that was just an oversight (luckily there wasn't many call sites like that), and we were lucky that this didn't cause observable negative effects. For example, I see conversions doing this.corLibrary.GetDeclaredSpecialType(SpecialType.System_Delegate), GetDeclaredSpecialType is supposed toi be called only on a core library.
We also should add an assert to ConversionsBase to verify that corLibrary is indeed a core library.

@AlekseyTs
Copy link
Contributor

@Youssef1313 Would it be possible to split unrelated changes into separate PRs?

@Youssef1313 Youssef1313 changed the title Reduce TypeConversions, bool arrays, and HashSet<TypeWithAnnotations> allocations Reduce TypeConversions allocations by caching them in AssemblySymbol Jul 17, 2023
@Youssef1313
Copy link
Member Author

@Youssef1313 Would it be possible to split unrelated changes into separate PRs?

Sure. #69069 #69070 #69071

@AlekseyTs AlekseyTs marked this pull request as draft July 18, 2023 05:00
@AlekseyTs AlekseyTs marked this pull request as ready for review July 18, 2023 05:03
AlekseyTs
AlekseyTs reviewed Jul 18, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/AssemblySymbol.cs Outdated
@@ -39,6 +40,8 @@ internal abstract class AssemblySymbol : Symbol, IAssemblySymbolInternal
/// </summary>
private AssemblySymbol _corLibrary;

private TypeConversions _lazyTypeConversions;
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

private TypeConversions _lazyTypeConversions;

I think this storage should be moved to MetadataOrSourceAssemblySymbol and to MissingCorLibrarySymbol. See how special types are cached in _lazySpecialTypes. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

@AlekseyTs I followed exactly what _lazySpecialTypes is doing, but I'm not very confident that the overrides where I have throw ExceptionUtilities.Unreachable() are indeed not reachable. Should I instead use CorLibrary.TypeConversions instead of those throw ExceptionUtilities.Unreachable()?

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 2)

AlekseyTs
AlekseyTs reviewed Aug 14, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/AssemblySymbol.cs Outdated
@@ -12,6 +12,7 @@
using System.Linq;
using System.Reflection;
using System.Reflection.PortableExecutable;
using System.Threading;
Copy link
Contributor

@AlekseyTs AlekseyTs Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this using necessary? #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 6)

AlekseyTs
AlekseyTs approved these changes Aug 16, 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 7)

@AlekseyTs
Copy link
Contributor

@cston, @dotnet/roslyn-compiler For the second review

@cston cston merged commit 87ea5af into dotnet:main Aug 16, 2023
25 checks passed
@ghost ghost added this to the Next milestone Aug 16, 2023
@cston
Copy link
Member

cston commented Aug 16, 2023

Thanks @Youssef1313.

@Youssef1313 Youssef1313 deleted the perf branch August 17, 2023 02:50
@allisonchou allisonchou mentioned this pull request Aug 22, 2023
Closed
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees
No one assigned
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.8 P2
Development

Successfully merging this pull request may close these issues.

3.1% allocations of TypeConversions
4 participants
@Youssef1313 @AlekseyTs @cston @dibarbet