Skip to content

Reduce amount of allocations related to tracking used assemblies:#41117

Merged
AlekseyTs merged 2 commits intodotnet:features/UsedAssemblyReferencesfrom
AlekseyTs:UsedAssemblies_12
Jan 24, 2020
Merged

Reduce amount of allocations related to tracking used assemblies:#41117
AlekseyTs merged 2 commits intodotnet:features/UsedAssemblyReferencesfrom
AlekseyTs:UsedAssemblies_12

Conversation

@AlekseyTs
Copy link
Copy Markdown
Contributor

  • Do not track usage of an assembly being built, it is always filtered out at the end.
  • Do not track dependencies when consumer is not interested in them. This mostly affects binding and lowering of an executable code.

@sharwell
Copy link
Copy Markdown
Contributor

sharwell commented Jan 21, 2020

@AlekseyTs let me know if you'd like me to set up and/or run AnalyzerRunner scenarios for getting concrete numbers. #Resolved

- Do not track usage of an assembly being built, it is always filtered out at the end.
- Do not track dependencies when consumer is not interested in them. This mostly affects binding and lowering of an executable code.
@AlekseyTs AlekseyTs requested review from a team, 333fred and jcouv January 22, 2020 16:03
@AlekseyTs AlekseyTs marked this pull request as ready for review January 22, 2020 16:04
@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented Jan 22, 2020

@dotnet/roslyn-compiler, @jcouv, @333fred Please review #Closed

return ToBadExpression(expr, resultKind);
}

public CompoundUseSiteInfo<AssemblySymbol> GetNewCompoundUseSiteInfo(BindingDiagnosticBag futureDestination)
Copy link
Copy Markdown
Member

@333fred 333fred Jan 22, 2020

Choose a reason for hiding this comment

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

Nit: it doesn't feel like this is the right location for this function. Perhaps in Binder.cs instead, given that it's a general helper? #Resolved

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had the same reaction. Does this have anything to do with value checks?


In reply to: 369822539 [](ancestors = 369822539)

@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented Jan 23, 2020

@dotnet/roslyn-compiler, @jcouv Please review, need a second sign-off. #Closed

1 similar comment
@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented Jan 23, 2020

@dotnet/roslyn-compiler, @jcouv Please review, need a second sign-off. #Closed

@jcouv jcouv self-assigned this Jan 23, 2020
return BoundCall.Synthesized(syntax, null, leftTruthOperator, loweredLeft);
}

private CompoundUseSiteInfo<AssemblySymbol> GetNewCompoundUseSiteInfo()
Copy link
Copy Markdown
Member

@jcouv jcouv Jan 24, 2020

Choose a reason for hiding this comment

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

nit: this method is also oddly situated. It's not related to binary operators. #Resolved

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Jan 24, 2020

I'm embarassed, because I didn't quite spot where we're reducing allocations.
Most of the changes are about initializing CompoundUseSiteInfo<AssemblySymbol> use site info structures with GetNewCompoundUseSiteInfo(diagnostics);, but that's a struct anyways. So I feel that I missed the forest in the middle of all those trees. #Resolved

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Jan 24, 2020

Hum, reading OP again, I think the optimization comes from use of the discard instances. Is that right? #Resolved

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 2)

@AlekseyTs
Copy link
Copy Markdown
Contributor Author

I think the optimization comes from use of the discard instances. Is that right?

The gain comes from the fact that we create instances that do not accumulate information we do not request.


In reply to: 577955908 [](ancestors = 577955908)

@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented Jan 24, 2020

I will address feedback on a separate PR in order to expedite an integration from master. #Closed

@AlekseyTs AlekseyTs merged commit 8182821 into dotnet:features/UsedAssemblyReferences Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants