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

Implement precompiled queries #33297

Merged
merged 3 commits into from
Apr 29, 2024
Merged

Implement precompiled queries #33297

merged 3 commits into from
Apr 29, 2024

Conversation

roji
Copy link
Member

@roji roji commented Mar 12, 2024

The precompiled query work is in good enough shape that I think it's the right moment to submit as a draft PR. There are still various test failures, checks and cleanups which I plan to implement, but it's more about edge cases at this point - the overall architecture should be sound. I'm happy to do a session with whoever wants to dive in and at least explain the general approach (we can also do a brown bag at some point if people want to).

Notes:

  • This PR is based on top of Introduce liftable constants to shaper to prepare for precompilation #32721 (liftable constants in shaper), which @maumar is working on (take care to filter out the 1st commit when reviewing). The shaper work must be complete for this PR to be completed.
  • One main reason to submit this now, is to allow @AndriySvyryd to react:
    • LinqToCSharpSyntaxTranslator now generates unsafe accessors when it encounters private field/property accesses; @AndriySvyryd I think this can be used instead of the previous "method access replacements" approach for compiled model generation, where the unsafe accessors are pre-generated externally and then substituted in the overriding implementation.
    • Work can begin on integrating this into MSBuild.
  • This is a pretty massive PR, implementing the bulk (and probably most complex parts) of precompiled queries; the more eyes on this, the better.

Closes #31331

@roji roji requested a review from a team March 12, 2024 15:16
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public async Task GeneratePrecompiledQueries(
Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly for @AndriySvyryd: this method is currently just hacked up to be invoked from e.g. an external console program, and needs to be adapted (in case it's not obvious). The precompiled query tests I've been working with call the method below, which just accepts a Compilation directly.

@@ -1824,30 +1827,33 @@ private void
out var createAndSetCollection,
out var createCollection);

// TODO
var unsafeAccessors = new HashSet<string>();
Copy link
Member Author

Choose a reason for hiding this comment

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

@AndriySvyryd note the unsafe accessors now getting generated inside LinqToCSharpSyntaxTranslators - ideally these would be used here instead of the current method replacements mechanism?

Copy link
Member

Choose a reason for hiding this comment

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

There are two reasons why I implemented them differently:

  1. Avoid generating duplicate accessors for the same members
  2. Support private property access

Copy link
Member Author

Choose a reason for hiding this comment

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

For the deduplication, this already occurs within the translator itself, so we could expose what's needed to allow deduplication across multiple invocations of the translator. For example, we could have the same instance of the translator accumulate unsafe accessor declarations internally as its reused, and at the end you'd all of them via a special API. Or we could externalize that state so you'd pass it back in, etc.

(For that state, originally I had a nice and clean dictionary from MemberInfo to the MethodDeclarationSyntax representing its interceptor - but needing different interceptors for field read/write makes that a tiny bit more complex.)

Private property access should also already be implemented in the translator - let me know if you see an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Using the same instance won't help when processing an assembly that doesn't contain the compiled model.

I was thinking that we can store memberAccessReplacements as an annotation in the compiled model and precompiled queries would use it. If you think that it's possible to have queries that access a private member that's not in the model then we can also keep unsafeAccessors

Copy link
Member Author

Choose a reason for hiding this comment

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

So FWIW, I looked again and the current implementation already deduplicates unsafe accessors across invocations (i.e. across shapers of different queries) - the state passed around is simply ISet<MethodDeclarationSyntax>, and at the end we dump that out into the generated file.

Using the same instance won't help when processing an assembly that doesn't contain the compiled model.

Let's do a call so I can understand this better (possibly tomorrow before/after the design meeting?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@AndriySvyryd for now I've left the these changes as they are in the PR, let's revisit and clean up after merge OK?

@roji roji force-pushed the PrecompiledQueries branch 5 times, most recently from d68d249 to 6b20e62 Compare March 25, 2024 22:00
@roji roji force-pushed the PrecompiledQueries branch 4 times, most recently from 165f114 to 53025c3 Compare April 7, 2024 17:30
Copy link
Member Author

@roji roji left a comment

Choose a reason for hiding this comment

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

I've more of less finished what I consider to be the most of the work here. There are some edge cases I know which aren't handled (and most probably some others I don't know), but I think it's in good enough shape to merge and fix local problems later.

So moving this out of draft state - it should be ready for a good review. Note that this is still based on top of #33351 which isn't yet done; be sure to exclude that when reviewing.

selectExpression.Orderings,
selectExpression.Limit,
selectExpression.Offset));
selectExpression.Offset,
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 that here - and below - I'm generally moving arround SelectExpression components to be in the order in which they're evaluated in SQL (i.e. offset before limit). We weren't always consistent, and the confusion can be hard to track down.

@@ -1824,30 +1827,33 @@ private void
out var createAndSetCollection,
out var createCollection);

// TODO
var unsafeAccessors = new HashSet<string>();
Copy link
Member Author

Choose a reason for hiding this comment

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

@AndriySvyryd for now I've left the these changes as they are in the PR, let's revisit and clean up after merge OK?

@roji roji marked this pull request as ready for review April 7, 2024 17:32
@cincuranet
Copy link
Contributor

@roji roji force-pushed the PrecompiledQueries branch 5 times, most recently from 185635c to c20e1b4 Compare April 8, 2024 21:13
@roji
Copy link
Member Author

roji commented Apr 8, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

src/EFCore.Design/Query/Internal/QueryLocator.cs Outdated Show resolved Hide resolved
@@ -56,7 +56,7 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="$(MicrosoftCodeAnalysisVersion)" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.10.0-2.final" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be probably fixed back before merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

This version contains an interceptor-related compiler fix which is required for the tests to run successfully - so I don't think we'll be able to change this back. However, it's a test-only dependency on a preview package, and 4.10.0 should release for 9.0, at which point hopefully we can align everything.

Copy link
Contributor

@cincuranet cincuranet Apr 9, 2024

Choose a reason for hiding this comment

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

I mean we can change the MicrosoftCodeAnalysisVersion variable. For 9.0 RTM everything will align fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, but this would limit the SDK versions that can be used by ordinary users with EF... We've had issues with changes like this in the past - given that we only need this in testing, I'd rather we left things as-is for now and update later as needed...

// containing type (and recursively, its containing types)
var typeTypeParameterMap = new Dictionary<string, Type>(GetTypeTypeParameters(methodSymbol.ContainingType));

IEnumerable<KeyValuePair<string, Type>> GetTypeTypeParameters(INamedTypeSymbol typeSymbol)
Copy link
Member

Choose a reason for hiding this comment

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

Move to the end and make static

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this can be static, since it invokes ResolveType... But moving to the end.

? ResolveType(typeSymbol)
: throw new InvalidOperationException("Could not find type symbol for: " + node);

private Type ResolveType(ITypeSymbol typeSymbol, Dictionary<string, Type>? genericParameterMap = null)
Copy link
Member

Choose a reason for hiding this comment

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

Review whether more calls of this should be done while supplying a genericParameterMap. For example, the expression could be referencing a generic type parameter from the containing type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I suspect there are a few bugs lurking around this... With everything going on, I'll defer this to now, and have added a note in #33639. We can fix later based on feedback or do a more thorough review.


// If the member isn't declared on the same type as the expression, (e.g. explicit interface implementation), add
// a cast up to the declaring type.
_ when member.Member.DeclaringType is Type declaringType && declaringType != member.Expression.Type
Copy link
Member

Choose a reason for hiding this comment

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

Add a check whether the declaring type is an interface or the member is hidden

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to only add the parentheses for interface-declared members - but what do you have in mind for hidden?

Copy link
Member

Choose a reason for hiding this comment

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

It's a corner case, but if the current type hides the member being used then we'd need to add a cast to the declaring type to be sure that the same member is used

Copy link
Member Author

@roji roji Apr 30, 2024

Choose a reason for hiding this comment

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

Makes sense - added note in #33639.

namespace System.Runtime.CompilerServices
{
[AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
sealed class InterceptsLocationAttribute : Attribute
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sealed class InterceptsLocationAttribute : Attribute
internal sealed class InterceptsLocationAttribute : Attribute

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should actually be file-scoped...


#nullable enable

public class CSharpToLinqTranslatorTest
Copy link
Member

Choose a reason for hiding this comment

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

Consider merging with LinqToCSharpSyntaxTranslatorTest as roundtrip tests

Copy link
Member Author

@roji roji Apr 29, 2024

Choose a reason for hiding this comment

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

IMHO it leaves things simpler for each test/test class to focus on one component here, given the complexity of the components...

Note also that the scope of each component here is very different - CSharpToLinq only needs to handle expression types supported by the compiler (i.e. only expression stuff, no loops/blocks/try catch), whereas LinqToCSharp needs to in theory handle everything (or at least everything we actually use in our shapers).

=> AssertExpression(
() => (object)new Blog(),
"(object)new CSharpToLinqTranslatorTest.Blog()");

Copy link
Member

Choose a reason for hiding this comment

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

Add tests for anonymous type, different closures, nested generics, indexers, records with

Copy link
Member Author

Choose a reason for hiding this comment

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

Added note in #33639. I hope we get to do a good bash on this before the release, and flush out bugs.

@roji roji merged commit ebbc5ee into dotnet:main Apr 29, 2024
5 of 7 checks passed
@roji roji deleted the PrecompiledQueries branch April 29, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use interceptors to redirect the query to precompiled code
4 participants