Fix #11464: avoid expanding local paramRef in HKTypeLambda #11465

liufengyun · 2021-02-18T21:24:24Z

Fix #11464: avoid expanding local paramRef in HKTypeLambda

smarter · 2021-02-18T21:29:22Z

Could you try re-enabling scissLucre in the community build? It was disabled a few months ago due to an unrelated problem in the community build (#10828 (comment)), and it fails on master with the Range ClassCastException

liufengyun · 2021-02-19T06:53:29Z

test performance please

dottybot · 2021-02-19T06:55:08Z

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

dottybot · 2021-02-19T08:07:42Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (9254ef6)

liufengyun · 2021-02-19T08:51:50Z

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

-          emptyRange
+          if variance == 1 then defn.AnyType
+          else if variance == -1 then defn.NothingType
+          else TypeBounds.empty


This is still not correct conceptually -- we need to know both the kind and variance of the LazyRef in order to produce an approximation.

liufengyun · 2021-02-20T07:59:51Z

test performance please

dottybot · 2021-02-20T08:00:42Z

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

dottybot · 2021-02-20T09:08:26Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (aa7c21e)

smarter · 2021-02-23T15:05:45Z

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

@@ -5724,6 +5724,37 @@ object Types {
            foldOver(xs, tp)
  end NamedPartsAccumulator

+  class ExistsLocalRef(using Context) extends TypeAccumulator[Boolean]:


Does this actually make type avoidance faster? It's not clear to me because this still requires traversing the type to see if there's something to avoid.

We don't have much empirical evidence. It avoids computing localSyms and it has shortcut for references to type or term parameters. Or we can enhance the TypeOps.avoid. This part can come later if there indeed a noticeable speedup. For now, I'll remove it from the PR.

liufengyun · 2021-03-02T13:04:28Z

test performance please

dottybot · 2021-03-02T13:04:35Z

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

dottybot · 2021-03-02T13:57:15Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (1dc4894)

Avoid unnecessary recursive types in the presence of LazyRef.

smarter · 2021-03-02T14:38:00Z

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

-        case tp: ThisType if toAvoid(tp.cls) =>
-          range(defn.NothingType, apply(classBound(tp.cls.classInfo)))
+        case tp: ThisType =>
+          tp


Could you explain why this change is needed and why it's safe to not do avoidance here?

We don't need this, this is an optimization -- the rationale is that ThisType doesn't exist outside of class, thus they are always safe in avoidance.

OK, I think this makes sense but it'd be good to check with an assert that this never happens in our tests (once the tests pass we can disable the assert for performance), and to add a comment explaining that this is safe.

smarter · 2021-03-02T15:40:16Z

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

-        case tp: ThisType if toAvoid(tp.cls) =>
-          range(defn.NothingType, apply(classBound(tp.cls.classInfo)))
+        case tp: ThisType =>
+          tp


OK, I think this makes sense but it'd be good to check with an assert that this never happens in our tests (once the tests pass we can disable the assert for performance), and to add a comment explaining that this is safe.

smarter · 2021-03-02T15:41:02Z

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

-        case tp: LazyRef if isExpandingBounds =>
-          emptyRange
+        case tp: LazyRef =>
+          if locals.contains(tp.ref) then tp


Why is it safe to return tp here, doesn't it need to be avoided if it contains a local symbol?

Here locals mean locally bound ParamRef in a HKTypeLambda -- we remember that in the set locals. The set is not the set of local symbols to be avoided.

Oh I see, would be good to give it another name to avoid confusing it with localSyms I think :). Maybe localParamRefs ?

smarter · 2021-03-02T15:55:30Z

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

@@ -401,6 +401,7 @@ object TypeOps:
  def avoid(tp: Type, symsToAvoid: => List[Symbol])(using Context): Type = {
    val widenMap = new ApproximatingTypeMap {
      @threadUnsafe lazy val forbidden = symsToAvoid.toSet
+      val locals = util.HashSet[Type]()


As an optimization, this could be an uninitialized var which is only initialized the first time we encounter an HKTypeLambda.

(or just make it a lazy val).

liufengyun · 2021-03-02T16:07:36Z

OK, I think this makes sense but it'd be good to check with an assert that this never happens in our tests (once the tests pass we can disable the assert for performance), and to add a comment explaining that this is safe.

@smarter Could you elaborate a little bit about the assertion? To clarify, ThisType of course can occur in avoidance, but they are safe because the ThisType must be a class that encloses the block to be avoided.

smarter · 2021-03-02T16:48:13Z

What I had in mind was to add an assert(!toAvoid(tp.cls)) in the ThisType clause to verify that indeed we don't have any ThisType to avoid.

liufengyun · 2021-03-02T18:43:30Z

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

+          // ThisType is only used inside a class.
+          // Therefore, either they don't appear in the type to be avoided, or
+          // it must be a class that encloses the block whose type is to be avoided.
+          assert(!toAvoid(tp.cls))
          tp


@smarter The CI confirmed the assertion. See https://github.com/lampepfl/dotty/actions/runs/614834038

liufengyun commented Feb 19, 2021

View reviewed changes

liufengyun self-assigned this Feb 19, 2021

liufengyun requested a review from smarter February 23, 2021 09:52

smarter reviewed Feb 23, 2021

View reviewed changes

liufengyun added the stat:do not merge label Feb 28, 2021

liufengyun force-pushed the fix-11464 branch from 134964f to a25ddf5 Compare March 2, 2021 12:39

liufengyun removed the stat:do not merge label Mar 2, 2021

liufengyun added 2 commits March 2, 2021 15:05

Fix scala#11464: avoid expanding local paramRef in HKTypeLambda

b987eeb

Enable pickling test

68f6ace

Avoid unnecessary recursive types in the presence of LazyRef.

liufengyun force-pushed the fix-11464 branch from 83bd989 to 68f6ace Compare March 2, 2021 14:06

liufengyun changed the title ~~Fix #11464: avoid leaking Range~~ Fix #11464: avoid expanding local paramRef in HKTypeLambda Mar 2, 2021

liufengyun requested a review from smarter March 2, 2021 14:26

smarter reviewed Mar 2, 2021

View reviewed changes

smarter requested changes Mar 2, 2021

View reviewed changes

smarter reviewed Mar 2, 2021

View reviewed changes

liufengyun commented Mar 2, 2021

View reviewed changes

Address review

5ff7b10

liufengyun force-pushed the fix-11464 branch from be4a2d4 to 5ff7b10 Compare March 2, 2021 18:47

smarter approved these changes Mar 2, 2021

View reviewed changes

liufengyun merged commit d7bbdcd into scala:master Mar 2, 2021

liufengyun deleted the fix-11464 branch March 2, 2021 20:14

Kordyjan added this to the 3.0.0 milestone Aug 2, 2023

Fix #11464: avoid expanding local paramRef in HKTypeLambda #11465

Fix #11464: avoid expanding local paramRef in HKTypeLambda #11465

Uh oh!

Conversation

liufengyun commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Feb 18, 2021

Uh oh!

liufengyun commented Feb 19, 2021

Uh oh!

dottybot commented Feb 19, 2021

Uh oh!

dottybot commented Feb 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liufengyun commented Feb 20, 2021

Uh oh!

dottybot commented Feb 20, 2021

Uh oh!

dottybot commented Feb 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liufengyun commented Mar 2, 2021

Uh oh!

dottybot commented Mar 2, 2021

Uh oh!

dottybot commented Mar 2, 2021

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!

liufengyun commented Mar 2, 2021

Uh oh!

smarter commented Mar 2, 2021

Uh oh!

liufengyun Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liufengyun commented Feb 18, 2021 •

edited

Loading

liufengyun Mar 2, 2021 •

edited

Loading