Fixes from benchmark#508
Conversation
LPTK
left a comment
LPTK
left a comment
There was a problem hiding this comment.
This PR is marked as ready, but it doesn't look like it is. Please document the ins and outs of your changes.
| def analyzeValues(asst: AssignInfo): Set[Value.RefLike] = | ||
| if emptyHanded && litValue === false then | ||
| analyzeAssignments(asst) | ||
| // analyzeAssignments(asst) |
There was a problem hiding this comment.
What's the rationale for commenting this? Does this lead to any missed opportunities?
There was a problem hiding this comment.
analyzeAssignment is just a no-op
There was a problem hiding this comment.
This code leads to a time blowup in compile test
There was a problem hiding this comment.
It would be really helpful if you could come up with a minimized reproduction of the problem where the blowup occurs and add it to the diff-tests.
There was a problem hiding this comment.
I found a test case that blow up even with this change. But this change is still beneficial.
There was a problem hiding this comment.
FWIW, here's the diagnosis from Codex, which seems correct:
Cause: branch merges were building a shared AssignInfo DAG, but several simplifier queries walked it as if it were a tree. Repeated if maybe do set x += 1 caused the same merged histories to be revisited exponentially, especially through assignedPureCallPrefix; value analysis and match-shape analysis had the same risk.
I don't like the fix proposed by Codex very much (it memoizes things with an ad-hoc IdentityHashMap); I think adding a lazy val in AssignInfo to replace the analyzeValues calls should be enough.
There was a problem hiding this comment.
I think I can't quite reproduce the problem I was finding with the current commented code. Maybe the issue is fixed in the hkmc2 branch already but the compile test were not terminating even at 60s before I commented those out. I don't think that's triggering this particular exponential problem since the problem went away immediately after I commented the analyzeAssignment out.
There was a problem hiding this comment.
nevermind I merged hkmc2 into my branch and can still reproduce the massive time difference between those commented out or not. Could be related to my TryCatch handling...
There was a problem hiding this comment.
After troubleshooting I confirmed the other issue is also an exponential blowup. But most likely some branches are pruned away completely when assignResult is commented out. So I think by fixing this case the assignResult should be somewhat fixed as well (at least not exponential, maybe quadratic).
2 participants
The benchmark have quite some outdated code and refuse to work again. This moves the Benchmark.mls file to the correct place (the old location was not compiled) and fixes a time limit blowup on BlockSimplifier.