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

Introduce liftable constants to shaper to prepare for precompilation #33351

Merged
merged 1 commit into from
Apr 12, 2024
Merged

Introduce liftable constants to shaper to prepare for precompilation #33351

merged 1 commit into from
Apr 12, 2024

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Mar 19, 2024

Part of #25009

json reader writer proper fix

@maumar maumar requested a review from roji March 19, 2024 01:44
@maumar
Copy link
Contributor Author

maumar commented Mar 19, 2024 •
edited
Loading

@roji created a separate PR to preserve the original code changes until we are happy with what we have. It's draft so far but should be ok to take a look. Only real outstanding issue is NTS, their value converter require SqlServerBytesReader/SqlServerBytesWriter, which in turn require NtsGeometryServices so it's a bit tricky to smuggle it in. Still a lot of small fixes, comments cleanups etc spoke too soon, still have lots of failures outside query

@maumar maumar force-pushed the aot_materializer branch 3 times, most recently from 5bd1bd8 to 3a7faf7 Compare March 24, 2024 11:35
@maumar maumar mentioned this pull request Mar 25, 2024
Closed
@maumar maumar force-pushed the aot_materializer branch 6 times, most recently from 3548892 to fed9a36 Compare March 27, 2024 22:45
@maumar maumar force-pushed the aot_materializer branch 2 times, most recently from d708d97 to 339b9af Compare March 28, 2024 06:49
@maumar maumar marked this pull request as ready for review March 28, 2024 06:49
@maumar maumar force-pushed the aot_materializer branch 2 times, most recently from dd1cf50 to 487225a Compare March 28, 2024 08:11
@maumar maumar requested a review from ajcvickers March 28, 2024 08:59
@maumar
Copy link
Contributor Author

maumar commented Mar 28, 2024

@ajcvickers specifically for changes in the EntityMaterializerSource and the area around (lazy loading, injectible service, parameter binding etc)

roji
roji requested changes Mar 31, 2024
View reviewed changes
Copy link
Member

@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.

Really nice to see this coming together! See first review comments.

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
src/EFCore/Query/StructuralTypeShaperExpression.cs Show resolved Hide resolved
src/EFCore/Storage/ValueConversion/BoolToStringConverter.cs Outdated Show resolved Hide resolved
src/EFCore/Storage/ValueConversion/ValueConverter.cs Show resolved Hide resolved
src/Shared/ExpressionExtensions.cs Show resolved Hide resolved
test/EFCore.Specification.Tests/Query/SpatialQueryTestBase.cs Outdated Show resolved Hide resolved
@maumar maumar force-pushed the aot_materializer branch 6 times, most recently from 226a7c9 to 8c3691b Compare April 4, 2024 08:11
@roji roji mentioned this pull request Apr 7, 2024
Merged
roji
roji reviewed Apr 7, 2024
View reviewed changes
src/EFCore.Relational/Query/RelationalQueryCompilationContext.cs Outdated Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated
// NOTE: this is unreliable way to get type mapping. Only doing this as last resort, hoping to "guess" the right one
typeMappingExpression = liftableConstantFactory.CreateLiftableConstant(
typeMapping,
c => (RelationalTypeMapping)c.Dependencies.TypeMappingSource.FindMapping(type, c.Dependencies.Model, null)!,
Copy link
Member

Choose a reason for hiding this comment

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

Doing the same lookup for the regular/inline case would at least show us if there's a failing test where this logic isn't correct (this is in general why I prefer we avoid having different paths for regular vs. precompiled as much as possible).

src/EFCore.SqlServer.HierarchyId/Storage/Json/SqlServerJsonHierarchyIdReaderWriter.cs Outdated Show resolved Hide resolved
ajcvickers
ajcvickers reviewed Apr 8, 2024
View reviewed changes
Copy link
Member

@ajcvickers ajcvickers left a comment

Choose a reason for hiding this comment

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

Changes to EntityMaterializerSource seem okay.

@maumar maumar force-pushed the aot_materializer branch 4 times, most recently from 828dbf0 to 36bb5a7 Compare April 9, 2024 06:04
@maumar
Copy link
Contributor Author

maumar commented Apr 9, 2024

@roji ready for the final peek, I removed all the changes not related to vanilla queries

roji
roji reviewed Apr 11, 2024
View reviewed changes
Copy link
Member

@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.

This looks almost ready - see remaining comments, I don't think they're blocking if they're problematic (let's discuss if so).

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
src/EFCore/Query/LiftableConstantExpressionHelpers.cs Outdated Show resolved Hide resolved
src/EFCore/Query/QueryCompilationContext.cs Show resolved Hide resolved
src/EFCore/Storage/Json/JsonBoolReaderWriter.cs Outdated Show resolved Hide resolved
src/EFCore/Storage/Json/JsonBoolReaderWriter.cs Outdated Show resolved Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs Outdated Show resolved Hide resolved
@maumar
Copy link
Contributor Author

maumar commented Apr 12, 2024

@roji updated with the latest feedback - all changes in separate commit for easier review

roji
roji approved these changes Apr 12, 2024
View reviewed changes
Copy link
Member

@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.

LGTM, thanks @maumar!

Take a look at the remaining unresolved comments for final minor cleanup, but otherwise good to go!

src/EFCore/Query/LiftableConstantProcessor.cs Outdated Show resolved Hide resolved
@maumar
Introduce liftable constants to shaper to prepare for precompilation.
7880f1c 
Fix materializer code to replace non-primitive constants with liftable constants.
Precompilation is opt-in in QueryCompilationContext, switched on for relational, off for everything else.

Architecture, design and initial implementation done by @roji, polish done by @maumar

Part of #25009
@maumar maumar merged commit f0e88d4 into main Apr 12, 2024
7 checks passed
@maumar maumar deleted the aot_materializer branch April 12, 2024 23:13
@roji roji mentioned this pull request Apr 13, 2024
Closed
@roji
Copy link
Member

roji commented Apr 13, 2024

@maumar after rebasing the precompiled query PR (#33297) on top of this merged on, that PR build fails because of unsupported constant nodes (specifically ParameterBindingInfo, it seems). Can you please take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajcvickers ajcvickers ajcvickers left review comments

@AndriySvyryd AndriySvyryd AndriySvyryd left review comments

@roji roji roji approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maumar @roji @ajcvickers @AndriySvyryd