-
Notifications
You must be signed in to change notification settings - Fork 323
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
Simplify compilation of nested patterns #4005
Simplify compilation of nested patterns #4005
Conversation
7e109ee
to
479d6d8
Compare
.../java/org/enso/interpreter/bench/benchmarks/semantic/NestedPatternCompilationBenchmarks.java
Outdated
Show resolved
Hide resolved
engine/runtime/src/test/scala/org/enso/compiler/test/pass/desugar/NestedPatternMatchTest.scala
Show resolved
Hide resolved
I've also noticed that the change shed a few seconds when running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The speed up results are great. The code changes look sane (as far as I can see).
@Benchmark | ||
public void sumList(Blackhole hole) throws IOException { | ||
// Compilation is included in the benchmark on purpose | ||
var module = ctx.eval(SrcUtil.source(benchmarkName, code)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we get more of "compilation benchmarks" then we should put them into their own directory/location. I assume they will fluctuate more than "fast path" ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, I agree
throw new PanicException( | ||
EnsoContext.get(this).getBuiltins().error().makeInexhaustivePatternMatch(object), this); | ||
if (fallthroughProfile.profile(isNested)) { | ||
return BranchResult.failure(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is nested, it returns failure in match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. This is so that the error does not propagate outside of the case expression.
7a1fdca
to
915e158
Compare
`NestedPatternMatch` pass desugared complex patterns in a very inefficient way resulting in an exponentional generation of the number of IR (and Truffle) nodes. Every failed nested pattern would copy all the remaining patterns of the original case expression. This change moves the fallthrough logic directly to Truffle nodes. That way we can generate much simpler IR for nested patterns. For ``` case x of Cons (Cons a b) Nil -> a + b Cons a Nil -> a _ -> 0 ``` we used to generate ``` case x of Cons w y -> case w of Cons a b -> case y of Nil -> a + b _ -> case x of Cons a z -> case z of Nil -> a _ -> case x of _ -> 0 _ -> 0 _ -> case x of Cons a z -> case z of Nil -> a _ -> case x of _ -> 0 _ -> 0 Cons a z -> case z of Nil -> a _ -> case x of _ -> 0 _ -> 0 ``` Now it is ``` case x of Cons w y -> case w of Cons a b -> case y of Nil -> a + b Cons a z -> case z of Nil -> a _ -> 0 ```
In the old implementation I had to kill the compiler after 15 minutes because it was still doing stuff in phases. Now the average execution is ~2 seconds or less.
Added tests that check the desugaring done in NestedPatternMatch pass.
915e158
to
4eaed76
Compare
Pull Request Description
NestedPatternMatch
pass desugared complex patterns in a very inefficient way resulting in an exponential generation of the number ofcase
IR (and Truffle) nodes. Every failed nested pattern would copy all the remaining patterns of the original case expression, in a desugared form. While the execution itself of such deeply nestedcase
expression might not have many problems, the time spent in compilation phases certainly was a blocker.This change desugars deeply nested into individual cases with a fallthrough logic. However the fallthrough logic is implemented directly in Truffle nodes, rather than via IR. That way we can generate much simpler IR for nested patterns.
Consider a simple case of
Before the change, the compiler would generate rather large IR even for those two patterns:
Now we generate simple patterns with fallthrough semantics and no catch-all branches:
Important Notes
If you wonder how much does it improve, then @radeusgd's example in https://www.pivotaltracker.com/story/show/183971366/comments/234688327 used to take at least 8 minutes to compile and run.
Now it takes 5 seconds from cold start.
Also, the example in the benchmark includes compilation time on purpose (that was the main culprit of the slowdown).
For the old implementation I had to kill it after 15 minutes as it still wouldn't finish a single compilation.
Now it runs 2 seconds or less.
Bonus points: This PR will also fix problem reported in https://www.pivotaltracker.com/story/show/184071954 (duplicate errors for nested patterns)
Checklist
Please include the following checklist in your PR:
Scala,
Java,
and
Rust
style guides.
./run ide build
and./run ide watch
.