[WIP] Quick-and-dirty speed-up for certain large switches #21981
Conversation
|
@CodaFi, @xedin, @davidungar, thoughts? Is this worth cleaning up and committing? |
|
@jrose-apple Without a test case demonstrating changed behavior how do we know the compact() algorithm has any effect? |
| /// Naive compacting of successive spaces that share a prefix. | ||
| /// | ||
| /// This turns DISJOINT((.a1, .b1), (.a1, .b2), (.a2, .b1)) into | ||
| /// DISJOINT((.a1, DISJOINT(.b1, .b2)), (.a2, .b1)). |
There was a problem hiding this comment.
This seems to be generally beneficial since identifying empty constructors is faster than disjuncts (single empty element in constructor makes whole constructor empty), which limits space explosion, right? (Sorry it's been a little while)
There was a problem hiding this comment.
Something like that. The main thing is that minus is expensive on constructors with multiple elements. In pseudocode:
> let s = (A, B) - (.a1, .b1)
s = (A - .a1, B) | (A, B - .b1)
> s - (.a1, .b2)
(A - .a1, B) | (A - .a1, B - .b1) | (A, B - .b1 - .b2)
This gets even worse with higher arity. With this change, we get:
> (A, B) - (.a1, .b1 | .b2)
(A - .a1, B) | (A, B - .b1 - .b2)
i.e. the exponential explosion is much slower.
Someone following along may have noticed another potential optimization: (A - .a1, B - .b1) in the first example is a subset of (A - .a1, B) and can therefore be dropped from the disjoint. We aren't checking that when forming disjoints but maybe we should.
|
@slavapestov It improves the test case I was working on in that Radar, just not enough. (It takes it from 40 seconds to 10 seconds or something like that, with our limit calibrated to be "about 1 second on a particular configuration, in terms of |
|
@jrose-apple Can you come up with a counter-based test then? |
|
Yeah, of course. I just wanted feedback on whether this was considered a good direction at all before I sunk any more effort into it. |
|
Closing in favor of #21999. I think this still improves things further but it's probably not worth the extra weight. |
I came up with this after seeing a particular switch over a 10x2x(5+3)-case tuple-of-enums that "timed out" (hit the
minuslimit), in rdar://problem/47365349. It didn't improve things enough, but it could still be useful until we (Robert?) get a chance to rewrite this matching.