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

Fix #320 - InvalidProgramException on .NET < 4.5.2 on x64 #687

Merged
merged 1 commit into from Feb 7, 2015

Conversation

Projects
None yet
6 participants
@nguerrera
Member

nguerrera commented Feb 7, 2015

In .NET 4.5 and .NET 4.5.1 on x64, the JIT would attempt to inline a
particular generic method, which would cause it to bail with an
InvalidProgramException.

Workaround the issue by applying MethodImplOptions.NoInlining to the
method. There is no perf loss because the whole purpose of this method
is to throw in the failure case and it is factored out precisely to
help the inlining of its callers.

The JIT bug has already been fixed in .NET 4.5.2, but we support
immutable collections on .NET 4.5 and .NET 4.5.1 as well, which is
why we must workaround the issue.

@nguerrera nguerrera force-pushed the nguerrera:workaround-320 branch from fdef977 to 7aa3c8d Feb 7, 2015

@nguerrera nguerrera changed the title from Fix #320 - InvalidProgramException on .NET 4.5 x64 to Fix #320 - InvalidProgramException on .NET < 4.5.2 on x64 Feb 7, 2015

@nguerrera

This comment has been minimized.

Member

nguerrera commented Feb 7, 2015

@nguerrera

This comment has been minimized.

Member

nguerrera commented Feb 7, 2015

Note about testing: simply running our unit tests with a release build on .NET 4.5 RTM x64 causes 79 failures due to this issue. However, our infrastructure is not setup to run in that configuration at the moment.

Fix #320 - InvalidProgramException on .NET < 4.5.2 on x64
In .NET 4.5 and .NET 4.5.1 on x64, the JIT would attempt to inline a
particular generic method, which would cause it to bail with an
InvalidProgramException.

Workaround the issue by applying MethodImplOptions.NoInlining to the
method. There is no perf loss because the whole purpose of this method
is to throw in the failure case and it is factored out precisely to
help the inlining of its callers.

The JIT bug has already been fixed in .NET 4.5.2, but we support
immutable collections on .NET 4.5 and .NET 4.5.1 as well, which is
why we must workaround the issue.

@nguerrera nguerrera force-pushed the nguerrera:workaround-320 branch from 7aa3c8d to 534c6e7 Feb 7, 2015

@ellismg

This comment has been minimized.

Contributor

ellismg commented Feb 7, 2015

LGTM. Sounds like a fun one to track down :-).

@nguerrera

This comment has been minimized.

Member

nguerrera commented Feb 7, 2015

Thanks, @ellismg. Yes, fun. :)

nguerrera added a commit that referenced this pull request Feb 7, 2015

Merge pull request #687 from nguerrera/workaround-320
Fix #320 - InvalidProgramException on .NET < 4.5.2 on x64

@nguerrera nguerrera merged commit 800a8af into dotnet:v1.0 Feb 7, 2015

1 check passed

default Merged build finished.
Details

@nguerrera nguerrera deleted the nguerrera:workaround-320 branch Feb 7, 2015

@stephentoub

This comment has been minimized.

Member

stephentoub commented Feb 7, 2015

LGTM. Nice work tracking it down.

@pharring

This comment has been minimized.

Contributor

pharring commented on 534c6e7 Feb 8, 2015

@AArnott I think you were the original author of this code. Have you seen this? Has it made its way into Microsoft.VisualStudio.Validation or Microsoft.VisualStudio.Utilities? Thankfully neither of those typically run in x64. I wonder what other methods might have this problem.

@nguerrera Do we need to scrub our sources for similar methods that fit the problematic pattern?

This comment has been minimized.

Member

nguerrera replied Feb 8, 2015

@pharring That method is relatively new. It was introduced in 4dde4af by @stephentoub.

I'm not sure if we need to scrub. I did not manage to determine the precise pattern, but I believe it involves something small enough to inline, shared generic code, and usage of typeof(T).

@pharring

This comment has been minimized.

Contributor

pharring commented Feb 8, 2015

@nguerrera OK, agreed. Usage of typeof(T) is rare enough.

@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016

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