Skip to content

Add UnsafeAccessor support for generic types#34146

Merged
AndriySvyryd merged 3 commits intomainfrom
ReenableCompiledModelAsserts
Jul 9, 2024
Merged

Add UnsafeAccessor support for generic types#34146
AndriySvyryd merged 3 commits intomainfrom
ReenableCompiledModelAsserts

Conversation

@AndriySvyryd
Copy link
Copy Markdown
Member

@AndriySvyryd AndriySvyryd commented Jul 3, 2024

  • Group UnsafeAccessor methods by target type
  • Generate fully qualified lambda signatures
  • Generate shorter UnsafeAccessor names
  • Reenable test code that uses the compiled model
  • Don't use Roslyn Formatter or Simplifier to make generation faster and avoid artifacts

Fixes #33773
Fixes #33470

@AndriySvyryd AndriySvyryd requested review from a team and roji July 3, 2024 06:43
@roji
Copy link
Copy Markdown
Member

roji commented Jul 4, 2024

@AndriySvyryd re removing Roslyn Formatter/Simplifier, can we first verify that these represent a significant part of the slowdown before removing them (/cc @maumar)? We of course don't have to produce nice code, but it's definitely a better if we can...

@roji
Copy link
Copy Markdown
Member

roji commented Jul 4, 2024

Another question: I don't remember any more where we landed, but do we actually use the unsafe accessors generated by LinqToCSharpTranslator? IIRC we've got the unsafe accessors you generate in the compiled model, ideally we'd just use those in the interceptors rather than generate them again...

Comment thread src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs
Comment thread src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs Outdated
Comment thread src/Shared/SharedTypeExtensions.cs
(InternalEntityEntry entry) => entry.ReadOriginalValue<int>(id, 0),
(InternalEntityEntry entry) => entry.ReadRelationshipSnapshotValue<int>(id, 0),
(ValueBuffer valueBuffer) => valueBuffer[0]);
int (InternalEntityEntry entry) => entry.ReadShadowValue<int>(0),
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.

This change is great, but just asking (@maumar) - is the slowness in generating the code, or in compiling it? Because this change helps the latter rather than the former.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I haven't had a chance to investigate this yet. Hopefully within next few days

Comment thread src/EFCore.Design/Design/Internal/CSharpHelper.cs
Comment thread src/EFCore.Design/Design/Internal/DbContextOperations.cs
@AndriySvyryd
Copy link
Copy Markdown
Member Author

@AndriySvyryd re removing Roslyn Formatter/Simplifier, can we first verify that these represent a significant part of the slowdown before removing them (/cc @maumar)? We of course don't have to produce nice code, but it's definitely a better if we can...

This change makes pre-compiled query generation for AOTSample100Queries take about 50% less time.

@AndriySvyryd
Copy link
Copy Markdown
Member Author

Another question: I don't remember any more where we landed, but do we actually use the unsafe accessors generated by LinqToCSharpTranslator? IIRC we've got the unsafe accessors you generate in the compiled model, ideally we'd just use those in the interceptors rather than generate them again...

We always use the unsafe accessors from the compiled model. There's an Assert checking that LinqToCSharpTranslator doesn't produce any new accessors

@AndriySvyryd AndriySvyryd force-pushed the ReenableCompiledModelAsserts branch from f87727b to 344cfc6 Compare July 6, 2024 02:41
Group UnsafeAccessor methods by target type
Generate fully qualified lambda signatures
Generate shorter UnsafeAccessor names
Reenable test code that uses the compiled model
Don't use Roslyn Formatter or Simplifier to make generation faster and avoid artifacts

Fixes #33773
Fixes #33470
Fixes #34057
@AndriySvyryd AndriySvyryd force-pushed the ReenableCompiledModelAsserts branch from 344cfc6 to d53a4fd Compare July 8, 2024 21:33
@roji
Copy link
Copy Markdown
Member

roji commented Jul 8, 2024

@AndriySvyryd re removing Roslyn Formatter/Simplifier, can we first verify that these represent a significant part of the slowdown before removing them (/cc @maumar)? We of course don't have to produce nice code, but it's definitely a better if we can...

This change makes pre-compiled query generation for AOTSample100Queries take about 50% less time.

... wow!

Copy link
Copy Markdown
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, but see comment about Generate/TryGenerate.

Comment thread src/EFCore.Design/Query/Internal/LinqToCSharpSyntaxTranslator.cs Outdated
@AndriySvyryd AndriySvyryd merged commit 4840bc3 into main Jul 9, 2024
@AndriySvyryd AndriySvyryd deleted the ReenableCompiledModelAsserts branch July 9, 2024 18:32
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.

Generate fully qualified lambda signatures for better compilation speed Generate shorter UnsafeAccessor names

4 participants