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

process more large expressions more systematically #6294

Merged
merged 19 commits into from
Mar 2, 2019
Merged

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Feb 27, 2019

This ensures we process large "linear" expressions using finite stack more systematically and adds some testing around this

  • Fix several cases where large inputs were not being processed using limited stack size

  • Add test cases capturing "reasonably large" samples for large inputs, e.. 200 or 500.

  • Add test cases for a "approximate maximum known currently accepted" large input, this is pretty much as large as I could get the compiler to accept in reasonable time, where we could still run the resulting .NET IL, give or take a bit. These represent our current limit of testing.

  • Systematically go through all recursive expression processing and make sure the main "linear" cases are treated correctly, see documentation of these cases in Add information about processing "linear" expressions fsharp/fsharp.github.io#95

#6258

  • The one remaining case which we intend to treat but don't is that very large conditional and "linear match" expressions are still not always generated correctly and can cause stack overflow during codegen. This is not easy to fix and can't be part of this PR. Instead I've added testing up to size 200 for these cases.

Future work (not part of this PR)

  • We should really go through and actually fix hard limits on various sizes of inputs. For example the compiler currently enforces a limit of size 500 for list expressions. We should really do the same for other repeatedly nested constructs.

  • Visual Studio seems to open all the "reasonably large" inputs but crashes on some of the "maxtested" inputs. We should add the above as test cases to each of the Visual Studio feature tests, e.g. for find-all-symbols, setting breakpoints, finding parameter info locations, anything that walks expressions

@dsyme
Copy link
Contributor Author

dsyme commented Feb 27, 2019

In parallel I'm adding documentation about processing large expressions to the compiler guide fsharp/fsharp.github.io#95

@dsyme dsyme changed the title process large expressions systematically process more large expressions more systematically Feb 28, 2019
@@ -2290,14 +2302,15 @@ and OptimizeLinearExpr cenv env expr contf =
Info = evalue' } ))

| LinearMatchExpr (spMatch, exprm, dtree, tg1, e2, spTarget2, m, ty) ->
let dtree, dinfo = OptimizeDecisionTree cenv env m dtree
let dtree', dinfo = OptimizeDecisionTree cenv env m dtree
Copy link
Contributor

Choose a reason for hiding this comment

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

Just recording this as an idle thought, gosh it'd be nice if we had the time to build a proper expression evaluator so that foo' identifiers and shadowed identifiers could get picked up in debug tools properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We should also just remove all use of foo' identifiers from the compiler

@cartermp
Copy link
Contributor

@dsyme Can you record the last two points as a tracking issue?

@cartermp
Copy link
Contributor

Also which files are the actual logic changing vs. refactorings?

@dsyme
Copy link
Contributor Author

dsyme commented Feb 28, 2019

@cartermp There are a number of fixes. There are about 7 places in the compiler where we traverse expresssions - a couple of these are generic map/fold operators. Each of these now handle the four important cases, e.g. here https://github.com/Microsoft/visualfsharp/pull/6294/files#diff-bd6897d64f42fb65f92f51d62c202fefR1807

These usually (not always) divert to the LinearExpr case which is explicitly tail recursive.
Normally this already existed and some logic has been hadded here: https://github.com/Microsoft/visualfsharp/pull/6294/files#diff-bd6897d64f42fb65f92f51d62c202fefR2315

Similarly

The "fold" for expressions is a bit tricker. We previously were returning an option from the exprIntercept (called to see if an expression has been rewritten), to indicate an intercept had happened. But this meant some things were not tail recursive. We now pass in an extra continuation to exprIntercept - one to call if an intercept happened one to call if it didn't. This means everything is tail recursive. See https://github.com/Microsoft/visualfsharp/pull/6294/files#diff-301467e77a66454fc34afc7b578f47baR6156. An example caller of this fold operation is here https://github.com/Microsoft/visualfsharp/pull/6294/files#diff-fab49007f5f1f6e6a25433328ade6631L47

Another change is that large sequences of if-then-else get type checked linearly, e.g. https://github.com/Microsoft/visualfsharp/pull/6294/files#diff-5b9ab9dd9d7133aaf23add1048742031L6051

@dsyme
Copy link
Contributor Author

dsyme commented Mar 1, 2019

Flakey test

[xUnit.net 00:00:28.28]     FSharp.Core.UnitTests.FSharp_Core.Microsoft_FSharp_Control.AsyncType.StartTask [FAIL]
Failed   FSharp.Core.UnitTests.FSharp_Core.Microsoft_FSharp_Control.AsyncType.StartTask
Error Message:
 System.InvalidOperationException : A task may only be disposed if it is in a completion state (RanToCompletion, Faulted or Canceled).

@dsyme dsyme closed this Mar 1, 2019
@dsyme dsyme reopened this Mar 1, 2019
@KevinRansom
Copy link
Member

@dsyme, this looks ready to me. Should I merge it?

@dsyme dsyme merged commit 166ec38 into dotnet:master Mar 2, 2019
@dsyme
Copy link
Contributor Author

dsyme commented Mar 2, 2019

Done, thanks

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.

3 participants