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

Restore fluent call depth #33907

Merged
merged 3 commits into from
Mar 7, 2019
Merged

Restore fluent call depth #33907

merged 3 commits into from
Mar 7, 2019

Conversation

jaredpar
Copy link
Member

@jaredpar jaredpar commented Mar 6, 2019

The change to move nullable to a two state solution inadverntently
changed how the CLR inlines a number of method calls. The combination of
not-inlining and over-inlining caused us to have a much larger "frame"
for analyzing a fluent call and exceeded our tolerance levels.

This adjusts the inlining so that we get the same results as before.

closes #33775

@@ -33,7 +33,7 @@ public void OverflowOnFluentCall()
numberFluentCalls = 460;
break;
case 32 when !isDebug:
numberFluentCalls = 1000;
numberFluentCalls = 1300;
Copy link
Member

Choose a reason for hiding this comment

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

1300 [](start = 40, length = 4)

nit: I think it will be possible to restore this to 1350 (and bump all other values by 50 as well). That would close #25606

Copy link
Member Author

Choose a reason for hiding this comment

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

Locally at least I can do that. Will see if it holds up in CI.

Copy link
Member

Choose a reason for hiding this comment

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

You could restore the other values as well (+50) to close the linked issue. Thanks


In reply to: 263190688 [](ancestors = 263190688)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!
Add "Closes #25606"?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's in the commit where the change was done. I put it there in case I had to revert that part of the change to get the PR through.

@jaredpar jaredpar force-pushed the fix-fluent branch 2 times, most recently from 85de151 to a8d9e39 Compare March 7, 2019 16:58
The change to move nullable to a two state solution inadverntently
changed how the CLR inlines a number of method calls. The combination of
not-inlining and over-inlining caused us to have a much larger "frame"
for analyzing a fluent call and exceeded our tolerance levels.

This adjusts the inlining so that we get the same results as before.

closes dotnet#33775
@jaredpar jaredpar marked this pull request as ready for review March 7, 2019 17:34
@jaredpar jaredpar requested a review from a team as a code owner March 7, 2019 17:34
@jaredpar
Copy link
Member Author

jaredpar commented Mar 7, 2019

@dotnet/roslyn-compiler for review.

Copy link
Member

@jcouv jcouv 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 (iteration 3)

@jcouv jcouv self-assigned this Mar 7, 2019
@jcouv jcouv added this to the 16.1.P1 milestone Mar 7, 2019
@jaredpar jaredpar merged commit 706351d into dotnet:master Mar 7, 2019
@jaredpar jaredpar deleted the fix-fluent branch March 7, 2019 19:08
prochan2 added a commit to postsharp/Metalama.Compiler that referenced this pull request Oct 17, 2023
prochan2 added a commit to postsharp/Metalama.Compiler that referenced this pull request Oct 17, 2023
prochan2 added a commit to postsharp/Metalama.Compiler that referenced this pull request Oct 18, 2023
…erride

dotnet#33907 Environment variable to customize the location of Metalama temp directory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow-up on stack using in fluent invocation binding
3 participants