Skip to content

C#: Improve performance of type conversion library#279

Merged
semmle-qlci merged 2 commits intogithub:masterfrom
hvitved:csharp/type-conversion-performance
Oct 9, 2018
Merged

C#: Improve performance of type conversion library#279
semmle-qlci merged 2 commits intogithub:masterfrom
hvitved:csharp/type-conversion-performance

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Oct 4, 2018

This PR applies the trick from Java's subtype implementation, namely to special case on the first two type arguments in constructed types.

For smaller snapshots, performance is mostly unchanged, but for huge snapshots it can make a big difference: For example, on an 8 MLoC customer database, the time it takes to construct the type conversion relation is reduced from ~40 minutes to ~4.5 minutes.

@calum

@hvitved hvitved added the C# label Oct 4, 2018
@hvitved hvitved requested a review from calumgrant October 4, 2018 11:11
@hvitved hvitved requested a review from a team as a code owner October 4, 2018 11:11
Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

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

Great stuff. Some very trivial comments.

* ```
* but performance is improved by explicitly evaluating the `i`th argument
* only when all preceding arguments are convertible.
private Type getTypeArgumentRanked(UnboundGenericType ugt, IdentityConvertibleConstructedType t, int i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A doc comment would be much appreciated here.

exists(int j |
fromTypeArgument = getTypeArgumentRanked(_, _, i) and
toTypeArgument = getTypeArgumentRanked(_, _, j) and
i <= j and j <= i
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was j introduced? (I guess it's an optimizer workaround but is it still needed, and what's it doing?) Perhaps a doc and raise a ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to avoid the optimizer joining on i.

}

pragma[nomagic]
private predicate convIdentityMultiple01Aux0(UnboundGenericType ugt, IdentityConvertibleConstructedType toType, TypeArgument fromTypeArgument0, TypeArgument toTypeArgument0, TypeArgument toTypeArgument1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, and elsewhere - max line width exceeded - sorry! But, would probably read more clearly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok to let the QL autoformatter handle this? I believe we should have that quite soon.

*/
pragma[nomagic]
private predicate convTypeArguments(TypeArgument fromTypeArgument, TypeArgument toTypeArgument, int i, TVariance v) {
exists(int j |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why introduce j for i (same as previous question)

@semmle-qlci semmle-qlci merged commit 2a9abcb into github:master Oct 9, 2018
@hvitved hvitved deleted the csharp/type-conversion-performance branch October 9, 2018 09:16
MathiasVP pushed a commit to MathiasVP/ql that referenced this pull request Nov 20, 2025
…te-name-matching

PS: Add predicates for case insensitive name matching on `Attribute`s
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants