More stack overflow fixes#7678
Conversation
TIHan
force-pushed
the
bug/linear-match-overflow-fix
branch
from
83bc428 to
0a6764a
Compare
|
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. |
| // 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 () = |
There was a problem hiding this comment.
Should add a comment describing why we are doing this.
|
Ok, CI passed. Surprising, also great. |
| 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) |
There was a problem hiding this comment.
This is just a refactoring. Will be useful if we want to delay generation of methods from lambdas.
|
@dsyme could you take a look at this? |
dsyme
left a comment
dsyme
left a comment
There was a problem hiding this comment.
I'll need to look at this really, really carefully in daylight, but I've left a few initial comments.
| 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<_>) = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
That's fine, I can do that.
| |> Seq.filter (fun (KeyValue(_, (_, isTargetPostponed))) -> isTargetPostponed) | ||
| |> List.ofSeq | ||
|
|
||
| let rec genRemaining remaining (queue: Queue<_>) = |
There was a problem hiding this comment.
Likewise here - let rec inside let rec inside let is just too much...
| 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)]) |
There was a problem hiding this comment.
This tail @ looks suspect (it is quadratic if decisions is long)
There was a problem hiding this comment.
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.
|
|
||
| /// Generate letrec bindings | ||
| and GenLetRecBindings cenv (cgbuf: CodeGenBuffer) eenv (allBinds: Bindings, m) = | ||
| let eenv = SetIsInLoop true eenv |
There was a problem hiding this comment.
Can you undo the renaming here to minimize the diff please?
There was a problem hiding this comment.
Sure, that's fair. I'll do the same to the other changes.
| sigToImplRemapInfo = [] (* "module remap info" *) | ||
| withinSEH = false | ||
| isInLoop = false } | ||
| inLoop = false } |
There was a problem hiding this comment.
Likewise undo this renaming to minimize the diff
| test997: string | ||
| test998: string | ||
| test999: string | ||
| test1000: string |
|
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. |
TIHan
force-pushed
the
bug/linear-match-overflow-fix
branch
from
31ba8b1 to
5b9126e
Compare
dsyme
left a comment
dsyme
left a comment
There was a problem hiding this comment.
Great work, much cleaner without the explicit stack!
4 participants
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.