Fix #3965: Make higher-kinded equality correct and efficient #3978

odersky · 2018-02-09T18:00:47Z

#3970 fixed equality for higher-kinded types but came at a steep performance penalty. This PR is trying to avoid the penalty.

Investigation showed that the main culprit was that equality reverted to eq for some ProtoType case classes that inherit from type. Since the multiple copies of these case class instances all had the same hashcode, hashtables saw a significant drop in performance. This shows the danger of redefining equality without also redefining hashcode at the same time. We now dropped the shortcut of having a global equals in Type, preferring to overwrite equals in each subclass where necessary.

To make detection of such issues easier in the future e9388bc adds the numbers of hash-cons table accesses and collisions in the statistics output.

odersky · 2018-02-09T18:01:38Z

Test performance please

liufengyun · 2018-02-09T19:14:59Z

test performance please

dottybot · 2018-02-09T19:16:00Z

performance test scheduled: 1 job(s) in queue, 1 running.

dottybot · 2018-02-09T21:24:08Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3978/ to see the changes.

Benchmarks is based on merging with master (5943331)

odersky · 2018-02-09T22:38:22Z

Test performance please

odersky · 2018-02-09T22:48:50Z

test performance please

dottybot · 2018-02-09T22:49:16Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2018-02-10T00:43:56Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3978/ to see the changes.

Benchmarks is based on merging with master (5943331)

odersky · 2018-02-10T10:40:57Z

test performance please

dottybot · 2018-02-10T10:41:45Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2018-02-10T12:36:04Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3978/ to see the changes.

Benchmarks is based on merging with master (5943331)

odersky · 2018-02-10T16:52:16Z

test performance please

dottybot · 2018-02-10T16:53:11Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2018-02-10T18:50:22Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3978/ to see the changes.

Benchmarks is based on merging with master (5943331)

odersky · 2018-02-10T21:53:29Z

It seems that caching MethodTypes and PolyTypes is not worth it, so we'll revert the last commit.

odersky · 2018-02-10T21:56:02Z

test performance please

dottybot · 2018-02-10T21:56:58Z

performance test scheduled: 1 job(s) in queue, 0 running.

liufengyun · 2018-02-11T04:05:46Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3978/ to see the changes.

Benchmarks is based on merging with master (5943331)

odersky · 2018-02-11T10:36:10Z

OK, I think performance is roughly on par with what it was before and I don't see any immediate ways to improve. So, ready for review.

smarter · 2018-02-14T17:27:26Z

Reduce number of calls to Type#equals

Is this useful at all given that it just forwards to referential equality?

smarter · 2018-02-14T17:27:58Z

compiler/src/dotty/tools/dotc/core/Types.scala

+    protected def iso(that: Any, bs: BinderPairs): Boolean = this.equals(that)
+
+    /** Equality used for hash-consing; uses `eq` on all recursive invocations,
+     *  except where a BindingType is inloved. The latter demand a deep isomorphism check.


typo: inloved -> involved

smarter · 2018-02-14T17:28:55Z

compiler/src/dotty/tools/dotc/core/Types.scala

+    final def equals(that: Any, bs: BinderPairs): Boolean =
+      (this `eq` that.asInstanceOf[AnyRef]) || this.iso(that, bs)
+
+    /** Is `this` isomorphic to that, using comparer `e`?


The e parameter doesn't exist anymore

smarter · 2018-02-14T17:30:20Z

compiler/src/dotty/tools/dotc/core/Types.scala

+     */
+    override def identityHash(bs: Binders) = {
+       def recur(n: Int, tp: BindingType, rest: Binders): Int =
+        if (this `eq` tp) finishHash(hashing.mix(hashSeed, n), 1)


Indentation is off, should be two characters but is only one

smarter · 2018-02-15T22:18:20Z

compiler/src/dotty/tools/dotc/core/Types.scala

+        else recur(n + 1, rest.tp, rest.next)
+      avoidSpecialHashes(
+        if (bs == null) System.identityHashCode(this)
+        else recur(1, bs.tp, bs.next))


Any reason to start at 1 and not 0?

I just don't know how a 0 behaves for hashing.mix

smarter · 2018-02-15T23:21:31Z

compiler/src/dotty/tools/dotc/core/Types.scala

@@ -1394,15 +1394,30 @@ object Types {
     */
    def simplified(implicit ctx: Context) = ctx.simplify(this, null)

+    final def equals(that: Any, bs: BinderPairs): Boolean =


It'd be good to list the laws or invariant relating the various equality related methods we now have, because it's getting rather complicated to follow. For example when iso is overridden, do we need override def equals(that: Any) = equals(that, null) ? I see it in some, but not all, cases.

smarter · 2018-02-15T23:50:02Z

compiler/src/dotty/tools/dotc/core/Types.scala

@@ -2774,7 +2835,7 @@ object Types {
  abstract case class MethodType(paramNames: List[TermName])(
      paramInfosExp: MethodType => List[Type],
      resultTypeExp: MethodType => Type)
-    extends CachedGroundType with MethodOrPoly with TermLambda with NarrowCached { thisMethodType =>
+    extends MethodOrPoly with TermLambda with NarrowCached { thisMethodType =>


Why is there no iso implementation for MethodType ?

Good point; we need one since MethodTypes can appear in refinements.

smarter · 2018-02-15T23:58:41Z

compiler/src/dotty/tools/dotc/core/Types.scala

-    override def equals(that: Any) = that match {
-      case that: ParamRef => binder.eq(that.binder) && paramNum == that.paramNum
+    override def iso(that: Any, bs: BinderPairs) = that match {
+      case that: ParamRef => binder.equalBinder(that.binder, bs) && paramNum == that.paramNum


Probably cheaper to do paramNum == that.paramNum first.

smarter · 2018-02-16T01:16:47Z

compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala

+      if (state.constraint contains tl) {
+      	var paramInfos = tl.paramInfos
+      	if (tl.isInstanceOf[HKLambda]) {
+      	  // HKLambdas care hash-consed, need to create an artificial difference by adding


typo: care -> are

Would something break if we did tl.clone() instead to get a different instance?

Yes, they would be assumed as equals, so e.g. pendingSubtypes would not distinguish between them.

smarter · 2018-02-16T01:23:02Z

compiler/src/dotty/tools/dotc/core/Types.scala

@@ -1394,15 +1394,30 @@ object Types {
     */
    def simplified(implicit ctx: Context) = ctx.simplify(this, null)

+    final def equals(that: Any, bs: BinderPairs): Boolean =
+      (this `eq` that.asInstanceOf[AnyRef]) || this.iso(that, bs)


I'm not completely convinced that the default equals should call iso. Why not explicitly use iso in the few cases where we need structural equality (monitored subtyping checks, constraint handing, anything else ?)

It's just a shorthand to avoid the repeated eq tests everywhere it is called.

Since correct hashing under binders seems to be very expensive (see performance data for scala#3970), let's try have fewer types that require this.

When compiling dotc/typer/*.scala we observe a reduction of the number of calls from ~3.7m to ~1.0m.

Now shows also total umber of accesses and hash collisions for each unique types set.

Dependent types RecType and HKLambda were generative. Any two such types were considered to be different, even if they had the same structure. This causes problems for subtyping and constraint solving. For instance, we cannot detect that a Hk-Lambda has already been added to a constraint, which can cause a cycle. Also, monitoredIsSubtype would not work if the compared types are dependent. Test cases are i3695.scala and i3965a.scala. To fix this, we need to have a notion of hashing and equality which identifies isomorphic HkLambdas and RecTypes. Since these are very frequently called operations, a lot of attention to detail is needed in order not to lose performance.

We need to add structural `equals` since MethodOrPoly's can be part of RefinedTypes.

odersky added the stat:do not merge label Feb 9, 2018

odersky force-pushed the fix-#3965-v3 branch from 01640ca to 19a14c5 Compare February 9, 2018 18:32

odersky force-pushed the fix-#3965-v3 branch from 5f22db6 to 03377c7 Compare February 10, 2018 10:15

odersky changed the title ~~Trying to isolate where performance loss comes from~~ Fix #3965: Make higher-kinded equality correct and efficient Feb 10, 2018

odersky removed the stat:do not merge label Feb 10, 2018

This was referenced Feb 10, 2018

Fix #3965: Trying to make higher-kinded equality correct and efficient [WIP] #3975

Closed

Fix #3965: Correct equality for higher-kinded types #3970

Closed

odersky assigned allanrenucci Feb 11, 2018

odersky requested a review from allanrenucci February 11, 2018 10:36

allanrenucci requested review from smarter and removed request for allanrenucci February 12, 2018 13:36

allanrenucci assigned smarter and unassigned allanrenucci Feb 12, 2018

smarter reviewed Feb 16, 2018

View reviewed changes

smarter assigned odersky and unassigned smarter Feb 16, 2018

odersky added 10 commits February 19, 2018 18:01

Make MethodTypes uncached

f00e883

Since correct hashing under binders seems to be very expensive (see performance data for scala#3970), let's try have fewer types that require this.

Add binders parameter to all hashcode computations

6290a65

Reduce number of calls to Type#equals

b74245f

When compiling dotc/typer/*.scala we observe a reduction of the number of calls from ~3.7m to ~1.0m.

Fix NamedPartsAccumulator

3e15796

Better stats for unique tables

e1a8588

Now shows also total umber of accesses and hash collisions for each unique types set.

Cache all method and polytypes

d4764c5

Revert: Cache all method and polytypes (reverted from commit c0e7113)

0c63930

Fix typo in diagnostics

cdfd219

Address reviewers comments

7cd930b

odersky force-pushed the fix-#3965-v3 branch from e96b8ca to 7cd930b Compare February 19, 2018 17:01

Fix MethodOrPoly

15644d6

We need to add structural `equals` since MethodOrPoly's can be part of RefinedTypes.

odersky merged commit 9053964 into scala:master Feb 20, 2018

Blaisorblade deleted the fix-#3965-v3 branch February 20, 2018 13:30

Fix #3965: Make higher-kinded equality correct and efficient #3978

Fix #3965: Make higher-kinded equality correct and efficient #3978

Uh oh!

Conversation

odersky commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Feb 9, 2018

Uh oh!

liufengyun commented Feb 9, 2018

Uh oh!

dottybot commented Feb 9, 2018

Uh oh!

dottybot commented Feb 9, 2018

Uh oh!

odersky commented Feb 9, 2018

Uh oh!

odersky commented Feb 9, 2018

Uh oh!

dottybot commented Feb 9, 2018

Uh oh!

dottybot commented Feb 10, 2018

Uh oh!

odersky commented Feb 10, 2018

Uh oh!

dottybot commented Feb 10, 2018

Uh oh!

dottybot commented Feb 10, 2018

Uh oh!

odersky commented Feb 10, 2018

Uh oh!

dottybot commented Feb 10, 2018

Uh oh!

dottybot commented Feb 10, 2018

Uh oh!

odersky commented Feb 10, 2018

Uh oh!

odersky commented Feb 10, 2018

Uh oh!

dottybot commented Feb 10, 2018

Uh oh!

liufengyun commented Feb 11, 2018

Uh oh!

odersky commented Feb 11, 2018

Uh oh!

smarter commented Feb 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odersky commented Feb 9, 2018 •

edited

Loading