-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add precompiled query generation to the dbcontext optimize command #33747
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
Conversation
881f8e9 to
b840cc6
Compare
620a708 to
0eaf687
Compare
| ContinueOnError="$(ContinueOnError)" | ||
| Condition="'$(PublishAot)'=='true'" | ||
| Properties="Configuration=$(Configuration);Platform=$(Platform);EFOptimizeContext=false;PublishAot=false" /> | ||
| Properties="Configuration=$(Configuration);Platform=$(Platform);PublishAot=false;_EFGenerationStage=$(_EFGenerationStage)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to always pass Configuration and Platform? Should we pass other properties?
| DependsOnTargets="_EFProcessGeneratedFiles" | ||
| Condition="'$(EFOptimizeContext)'=='true'" | ||
| Condition="'$(_EFGenerationStage)'==''" | ||
| Inputs="$(MSBuildAllProjects); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way of determining whether CoreCompile is going to be skipped other than duplicating its inputs and outputs?
|
|
||
| MSBuildLocator.RegisterDefaults(); | ||
| // TODO: pass through properties | ||
| var workspace = MSBuildWorkspace.Create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What properties should we pass through from the MsBuild invocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I'm not sure... I'm assuming it's reasonable to expect the project to be buildable as-is (without properties), but the user may have tweaked their project with various properties...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it's buildable the code might contain #ifdefs that significantly alter the logic based on DefineConstants.
3d181c1 to
b067140
Compare
|
Just to say that this is on my list to review... Will work up to it in the next few days. |
Ok. Just note that the longer you take to review the more code will need to be reviewed 😉 |
|
That's a pretty effective way to speed up a code review :) |
roji
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the MSBuild Tasks part is a total 🐑 🇮🇹 though :) Is there someone who knows this part a bit (@ajcvickers)?
Just to confirm: after this PR, the precompiled query code will use the unsafe accessors generated into the compiled model, rather than generate its own (via the code I wrote in LinqToCSharpSyntaxTranslator), right? That would be a good thing, just making sure.
|
|
||
| MSBuildLocator.RegisterDefaults(); | ||
| // TODO: pass through properties | ||
| var workspace = MSBuildWorkspace.Create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I'm not sure... I'm assuming it's reasonable to expect the project to be buildable as-is (without properties), but the user may have tweaked their project with various properties...
src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs
Outdated
Show resolved
Hide resolved
| """ | ||
| ); | ||
|
|
||
| var name = Uniquifier.Uniquify( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also uniquify only at the very top, i.e. before actually dumping the files to disk, rather than dealing with it at each point where we create a ScaffoldedFile... In that sense the Name would only be the "desired" name (or a hint).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uniquifier needs to be before the suffix (e.g. .g) and before the extension. If we do it at the top then we'd either need to parse the name or make ScaffoldedFile to contain all the building parts of the name separately.
Another problem is that even if the filenames are uniquified, the class names could still be clashing. I haven't solved this yet, but it would need to be done at this level anyway.
src/EFCore.Tasks/buildTransitive/Microsoft.EntityFrameworkCore.Tasks.props
Outdated
Show resolved
Hide resolved
|
I would find it very helpful if there were a Markdown doc describing what this is doing, ideally with some Mermaid diagrams. I was going to try to document it myself but I'm getting lost in the details. Can you share a binlog of a project building/publishing with this stuff turned on that I can look at? |
Yes, and I added an |
|
@rainersigwald There are basically two flows:
For Publish
|
Am I understanding correctly that this means you must first build the whole solution (or a meaningful subset), then generate code, then rebuild all of the assemblies that had code generated? |
Yes. And it's even worse when |
|
BTW @AndriySvyryd consider copy-pasting some version of the explanation above into the targets themselves for future us. |
…unless a single one is specified Fixes #33558
Also adds publish integration for precompiled queries and combines it with compiled model generation.
For the compiled model and precompiled queries to be generated when publishing with NativeAOT the only action needed is to reference
Microsoft.EntityFrameworkCore.Tasksfrom all projects containing aDbContextor a query.For solutions where specifying the startup project is necessary,
EFStartupProjectshould be set.EFOptimizeContextcan be set totrueto enable code generation outside of NativeAOT.EFScaffoldModelStageandEFPrecompileQueriesStagecan be set to eitherpublishorbuildto control at what stage will the code be generated. Any other value will disable the corresponding generation (in case the code is generated manually usingdotnet ef dbcontext optimize)If there's more than one context and
DbContextNameis not set, then the compiled model will be generated for all of them.EFTargetNamespaceandEFOutputDircan be used to further fine-tune the generation.Fixes #33103
Fixes #33558