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

Avoid infinite recursive generics (Fixes #20393) #20494

Closed
wants to merge 1 commit into from
Closed

Avoid infinite recursive generics (Fixes #20393) #20494

wants to merge 1 commit into from

Conversation

hez2010
Copy link
Contributor

@hez2010 hez2010 commented Apr 2, 2020

Avoid infinite recursive generics

  • The code builds and tests pass (verified by our automated build checks)

  • Commit messages follow this format
    Summary of the changes
    - Detail 1
    - Detail 2

      Fixes #bugnumber
    
  • Tests for the changes have been added (for bug fixes / features)

  • Code meets the expectations our engineering guidelines.
    https://github.com/dotnet/aspnetcore/wiki/Engineering-guidelines

Fixes #20393

Also I think this fix can be backport to branch release/3.1

- Avoid `TResult` = `ExecutionResult<TResult>` call for `VerifySucceeded`

Fixes #20393
@hez2010 hez2010 changed the title Avoid infinite recursive generics (#20393) Avoid infinite recursive generics (Fixes #20393) Apr 2, 2020
@ajcvickers
Copy link
Member

@hez2010 Thanks for the PR. Given that this is a perf issue, can you show the testing you did to ensure that the perf after the change is better than the perf before the change?

Also, it's extremely unlikely that this will be ported to 3.1.x. See release planning for details on how we decide which issues to patch.

@hez2010
Copy link
Contributor Author

hez2010 commented Apr 2, 2020

Before:

Method Mean Error StdDev
TestExecutionStrategy 2.149 us 0.0363 us 0.0322 us

After:

Method Mean Error StdDev
TestExecutionStrategy 2.087 us 0.0314 us 0.0278 us

This is not about runtime performance but about generated native image size.
Before this PR, ExecuteImplementation will call itself with TResult=ExecutionResult<TResult> and made AOT compilers generate more codes for it, each TResult will get an implementation of ExecuteImplementation<TState, TResult>, which will even cause infinite generics recursion.

@ajcvickers
Copy link
Member

@hez2010 We discussed this and came to the following conclusions:

  • The new code is less maintainable than the original code
  • Currently measured perf improvements are marginal
  • If AOT changes these perf characteristics, then we need to see those numbers
    • If this means the AOT code needs to updated not to throw before we can get these numbers, then we are happy to wait.

roji added a commit to roji/efcore that referenced this pull request Apr 26, 2020
Alternative to dotnet#20494
@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main July 23, 2020 04:35
@AndriySvyryd
Copy link
Member

Closing this PR as this isn't the way we plan to resolve the issue

@darkguy2008
Copy link

@AndriySvyryd sorry to chip in, but what's the way to solve the issue then?

I'm currently facing the issue mentioned above ( dotnet/runtimelab#283 ) where there isn't a performance or even binary size issue as @hez2010 states. A simple project just doesn't compile, so IMHO the code doesn't look that bad either (and I have rarely worked with Core's source code, go figure). I agree that it can be refactored, as it repeats, but it's a fix to a big problem because any project that includes EFCore and tries to compile with CoreRT just doesn't even build as the process gets stuck in an infinite stackoverflow exception (under Linux) and then crashes (under Windows).

I would've been even more worried if there wasn't a fix, but the fix is here... and it was rejected ?? This just doesn't make any sense. What would be the proper way to fix this, then? It's an error, a known error, and there's a proposed fix. Maybe even accepting it and then refactoring the code could've been a better idea, I guess.

@roji
Copy link
Member

roji commented Nov 14, 2020

@darkguy2008 I haven't reviewed this PR, but I can only say that I do intend to look at this and probably remove instances of infinite recursive generics in EF for 6.0 (unfortunately there was no time to look into this for 5.0). This isn't a comment on this specific PR and what it does to get rid of the infinite recursion.

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Nov 15, 2020

A better way of fixing the issue would be to change the signature of ExecuteImplementation in a way that allows recursive calls to be made using the same generic arguments. For this operation and verifySucceeded should have the same type.

roji added a commit to roji/efcore that referenced this pull request Feb 22, 2021
Alternative to dotnet#20494
roji added a commit to roji/efcore that referenced this pull request Feb 22, 2021
Alternative to dotnet#20494
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.

Infinite generic expansions cause efcore perform poorly with AOT compilation
5 participants