Fix pathologically slow compilation of some pattern matches since 2.13.7 #10258
Conversation
|
This seems to non-deterministically fail |
|
I feel the same, passed/failed on local randomly. Which should I do to fix it?
|
|
@KisaragiEffective I don't mind if we switch to java.util.LinkedHashSet. |
|
I'm not sure why it was failed... |
| // would be nice to statically check whether a prop is equational or pure, | ||
| // but that requires typing relations like And(x: Tx, y: Ty) : (if(Tx == PureProp && Ty == PureProp) PureProp else Prop) | ||
| final case class And(ops: Set[Prop]) extends Prop | ||
| object And { | ||
| def apply(ps: Prop*) = create(ps) | ||
| def create(ps: Iterable[Prop]) = ps match { | ||
| case ps: Set[Prop] => new And(ps) | ||
| case _ => new And(ps.to(scala.collection.immutable.ListSet)) | ||
| case _ => new And(new java.util.LinkedHashSet[Prop](ps.toSet.asJava).asScala.to(Set)) |
There was a problem hiding this comment.
ps.toSet creates an immutable HashSet which has non-deterministic iteration order.
I think we can use Scala's mutable.LinkedHashSet instead of Java's. Maybe the best option is to change the And and Or case classes to have a mutable.LinkedHashSet[Prop] parameter? That would express the fact that an ordered collection is required.
Otherwise, use collection.Set[Prop]?
@dwijnand what do you think?
There was a problem hiding this comment.
Isn't java.util.LinkedHashSet known to be faster than mutable.LinkedHashSet? Good catch on the toSet: we need to build up the set in insertion-order of the iterable.
I don't mind if we change the parameter either.
There was a problem hiding this comment.
The Java version is possibly still faster, but our LinkedHashSet was recently brought up to date (#10221). Compared to ListSet which we are replacing here, it's likely going to be fine.
|
@KisaragiEffective interested in returning to this...? |
|
It's still failed on t7020. Are there other implicit assumptions? |
|
The current version still calls |
|
they're green except combined step 🎉 |
* experiment fix for SI-12499 (patmat perf) * use java.util.LinkedHashSet * switch j.u to s.c.m LinkedHashSet * avoid convert to Set to keep deterministic iteration order * fix compile error Co-authored-by: Dale Wijnand <dale.wijnand@gmail.com>
KisaragiEffective
changed the title
|
(changed title according to squashed commit) |
SethTisue
changed the title
|
@SethTisue IMO the issue is only partially fixed; see my comment in |
|
@michelou ah, sorry to hear that. I wonder if what you're experiencing now has some entirely different cause, or if it's more that the PR we have needs a followup adjustment. |
|
Seems this is not enough for solve problem, see flix/flix#4916 (comment) |
|
I see — new ticket, if you're able to minimize it? |
|
I've already filed (however not sure if it's root cause of the perf regression) scala/bug#12760 |
by switching the pattern matcher to use
mutable.LinkedHashSetinstead ofListSetfixes scala/bug#12499