More stack overflow fixes #7678

TIHan · 2019-10-02T01:27:08Z

To resolve this: #7673

What I'm curious about is the fact that the generated equality/comparison methods on the newly added tests are not broken apart into multiple methods. Effectively, these methods become very very large which probably isn't a good thing.

The initial fix addresses part of the issue with linear pattern matching combined with let bindings, but it still isn't enough. If these methods were broken apart, then the fix wouldn't need to be in IlxGen. But, this fix also breaks other stuff, so it isn't complete unfortunately.

Instead of trying to do this in IlxGen, we should break apart methods, which that is dictated by the optimizer I believe.

In regards to this specific issue, #7673, it can technically be resolved if we took out the stack guard. The stack guard will throw an exception earlier than an actual stack overflow, so the record type just happened to be in the sweet spot where the guard would throw but an overflow still wouldn't occur.

TIHan · 2019-10-31T00:45:45Z

I've taken the approach of linearizing Expr.Match as a whole in IlxGen by using a queue and continuations. This is quite different from how Expr.Match is handled in other parts of the compiler, so we'll see how it works out. This could technically increase allocations when compiling to an assembly, but it may not be as bad as we think.

If this approach fails, the fallback is to mitigate the original user issue is: split non-user defined methods even when optimizations are turned off. Though, we will still have an issue with large structs as their generated methods do not get split due to byref free vars.

TIHan · 2019-10-31T00:57:51Z

src/fsharp/IlxGen.fs

+            //        Since code is branching and joining, the cgbuf stack is maintained manually.
+            let targetQueue = GenDecisionTreeAndTargets cenv cgbuf stackAtTargets eenv tree targets repeatSP sequelOnBranches
+
+            let rec popTargetQueue () =


Should add a comment describing why we are doing this.

TIHan · 2019-10-31T03:15:01Z

Ok, CI passed. Surprising, also great.

TIHan · 2019-10-31T03:17:12Z

src/fsharp/IlxGen.fs

-        CountClosure()
-        for cloTypeDef in cloTypeDefs do
-            cgbuf.mgbuf.AddTypeDef(ilCloTypeRef, cloTypeDef, false, false, None)
+        GenMethodForLambda cenv cgbuf.mgbuf eenv (entryPointInfo, cloinfo, eenvinner, body, isLocalTypeFunc, m)


This is just a refactoring. Will be useful if we want to delay generation of methods from lambdas.

cartermp · 2019-10-31T17:53:35Z

@dsyme could you take a look at this?

dsyme

I'll need to look at this really, really carefully in daylight, but I've left a few initial comments.

dsyme · 2019-10-31T22:37:50Z

src/fsharp/IlxGen.fs

-    let targetInfos = GenDecisionTreeAndTargetsInner cenv cgbuf None stackAtTargets eenv tree targets repeatSP (IntMap.empty()) sequel
-    GenPostponedDecisionTreeTargets cenv cgbuf stackAtTargets targetInfos sequel
+and GenDecisionTreeAndTargets cenv cgbuf stackAtTargets eenv tree targets repeatSP sequel : (Queue<unit -> (IlxGenEnv * EmitSequencePointState * Expr * sequel) option>) =
+    let rec genDecisions targetInfos decisions (queue: Queue<_>) =


Please don't use any inner recursive functions in IlxGen.fs. They are always suspect w.r.t. readability, and IlxGen.fs is written without their use because there are enough side effects going on that it's plainer and simpler to do without them. Take it to the module scope and call it GenDecisionTreeAndQueuedTargets or something

That's fine, I can do that.

dsyme · 2019-10-31T22:39:57Z

src/fsharp/IlxGen.fs

+                |> Seq.filter (fun (KeyValue(_, (_, isTargetPostponed))) -> isTargetPostponed)
+                |> List.ofSeq
+
+            let rec genRemaining remaining (queue: Queue<_>) =


Likewise here - let rec inside let rec inside let is just too much...

dsyme · 2019-10-31T22:41:46Z

src/fsharp/IlxGen.fs

-            GenDecisionTreeAndTargetsInner cenv cgbuf (Some caseLabel) stackAtTargets eenv caseTree targets repeatSP targetInfos sequel)
-    targetInfos
+    (decisions, caseLabels, cases) 
+    |||> List.fold2 (fun decisions caseLabel (TCase(_, caseTree)) -> decisions @ [(Some caseLabel, eenv, caseTree)])


This tail @ looks suspect (it is quadratic if decisions is long)

Yea, I can see that. Actually, I should have just used List.map here, and would append the 1 default decision right before this. I think that would be fine.

dsyme · 2019-10-31T22:42:20Z

src/fsharp/IlxGen.fs

@@ -5150,7 +5196,7 @@ and GenLetRecFixup cenv cgbuf eenv (ilxCloSpec: IlxClosureSpec, e, ilField: ILFi

 /// Generate letrec bindings
 and GenLetRecBindings cenv (cgbuf: CodeGenBuffer) eenv (allBinds: Bindings, m) =
-    let eenv = SetIsInLoop true eenv


Can you undo the renaming here to minimize the diff please?

Sure, that's fair. I'll do the same to the other changes.

dsyme · 2019-10-31T22:42:40Z

src/fsharp/IlxGen.fs

@@ -7535,7 +7581,7 @@ let GetEmptyIlxGenEnv (ilg: ILGlobals) ccu =
      innerVals = []
      sigToImplRemapInfo = [] (* "module remap info" *)
      withinSEH = false
-      isInLoop = false }
+      inLoop = false }


Likewise undo this renaming to minimize the diff

dsyme · 2019-10-31T22:43:15Z

tests/fsharp/Compiler/Stress/LargeExprTests.fs

+        test997: string
+        test998: string
+        test999: string
+        test1000: string


dsyme · 2019-10-31T22:44:28Z

@cartermp @TIHan remind me if I neglect to look at this tomorrow....

TIHan · 2019-11-01T02:57:28Z

Thank you @dsyme .

I made more adjustments.

I also realized that I didn't need to use a queue... I could just keep using CPS on the decision tree functions.

…overflow-fix

dsyme

Great work, much cleaner without the explicit stack!

TIHan added 3 commits October 25, 2019 13:06

Initial work

1e1be1b

Quick re-name for OptimizeMethod(s) to OptimizeObjectExprMethod(s).

2b1e29c

Delaying lambdas working

0a6764a

TIHan force-pushed the bug/linear-match-overflow-fix branch from 83bc428 to 0a6764a Compare October 28, 2019 19:30

TIHan added 5 commits October 28, 2019 13:33

fix regression test, starting to add linear match expr handling

4df0e4f

Minor code move

bf04355

Might have it working

99a2ca2

Fixed issue not capturing locals

f3ab72c

Cleanup, tests pass

95d8f1b

TIHan changed the title ~~[WIP] More stack overflow fixes~~ More stack overflow fixes Oct 31, 2019

TIHan added 2 commits October 30, 2019 17:27

We do not need this anymore

a1e04ef

Remove commented code

ee9a677

TIHan commented Oct 31, 2019

View reviewed changes

TIHan added 3 commits October 30, 2019 18:30

Changed test so coreclr can pass

f166d6e

Do not delay lambda generation for now

d22a018

Get rid of optimize flag in test

1fe8051

TIHan commented Oct 31, 2019

View reviewed changes

cartermp requested a review from dsyme October 31, 2019 17:53

dsyme reviewed Oct 31, 2019

View reviewed changes

TIHan added 6 commits October 31, 2019 18:08

Removed unrelated code changes to the fix

eb333d9

Removed rest of unrelated code changes to fix

02b41ce

Removed quadratic append

8ddce91

Moved local rec functions into top module

74196ae

Added GenDecisionTreeTargetQueue

e6baf7f

Instead of using a queue, use CPS through other functions

8dfcc6a

TIHan added 2 commits October 31, 2019 20:09

Minor cleanup

b663ffc

Pure CPS

b5a9996

TIHan mentioned this pull request Nov 2, 2019

FreeVars cache optimization on Expr.Let #7809

Merged

Merge remote-tracking branch 'upstream/master' into bug/linear-match-…

5b9126e

…overflow-fix

TIHan force-pushed the bug/linear-match-overflow-fix branch from 31ba8b1 to 5b9126e Compare November 3, 2019 10:53

dsyme approved these changes Nov 6, 2019

View reviewed changes

KevinRansom merged commit 1cd5f72 into dotnet:master Nov 6, 2019

cartermp mentioned this pull request Nov 27, 2019

FSI 4.7: expression is too large and/or complex to emit #7673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More stack overflow fixes #7678

More stack overflow fixes #7678

TIHan commented Oct 2, 2019 •

edited

Loading

TIHan commented Oct 31, 2019 •

edited

Loading

TIHan Oct 31, 2019

TIHan commented Oct 31, 2019

TIHan Oct 31, 2019

cartermp commented Oct 31, 2019

dsyme left a comment

dsyme Oct 31, 2019

TIHan Nov 1, 2019

dsyme Oct 31, 2019

dsyme Oct 31, 2019

TIHan Nov 1, 2019

dsyme Oct 31, 2019

TIHan Nov 1, 2019

dsyme Oct 31, 2019

dsyme Oct 31, 2019

dsyme commented Oct 31, 2019

TIHan commented Nov 1, 2019

dsyme left a comment

More stack overflow fixes #7678

More stack overflow fixes #7678

Conversation

TIHan commented Oct 2, 2019 • edited Loading

TIHan commented Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

TIHan commented Oct 31, 2019

Choose a reason for hiding this comment

cartermp commented Oct 31, 2019

dsyme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented Oct 31, 2019

TIHan commented Nov 1, 2019

dsyme left a comment

Choose a reason for hiding this comment

TIHan commented Oct 2, 2019 •

edited

Loading

TIHan commented Oct 31, 2019 •

edited

Loading