don't generate unneeded references to outer objects in inner classes

[scabug]

Currently (change set 16246 of trunk) named inner classes always contain a reference to their enclosing object, regardless of whether it is use or not. This is similar to the old behavior of anonymous inner classes as demonstrated in https://lampsvn.epfl.ch/trac/scala/ticket/854 . I think named inner classes should not retain a reference to the enclosing object if it is not required so that subtle memory leaks can be avoided.

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-1419?orig=1
Reporter: @eengbrec
See #4440

[scabug]

@stepancheg said:
-1 for this. If you don't need reference to outer class, declare your inner class outside.

[scabug]

@stepancheg said:
I've read comments to #854, and now I'm not sure.

[scabug]

@eengbrec said:
Replying to [comment:2 stepancheg]:
What would be the harm in changing the behavior? I think changing it would lead to less surprises, which is almost always a good thing.

[scabug]

@stepancheg said:
Replying to [comment:3 eengbrec]:

val m = new WeakHashMap[A, Something]

class A {
  class X
  def f = new X
}

def g = {
val a = new A
m += (a, someValue)
a.f
}

someValue will be garbage collected if X has no reference to outer class.

[scabug]

@stepancheg said:
Replying to [comment:3 eengbrec]:

val m = new WeakHashMap[A, Something]

class A {
  class X
  def f = new X
}

def g = {
  val a = new A
  m += (a, someValue)
  a.f
}

someValue will be garbage collected if X has no reference to outer class.

[scabug]

@eengbrec said:
Replying to [comment:5 stepancheg]:
Where is "someValue" declared? I can't tell when it would be GC'd without knowing where the references to it are.

That being said, isn't the point of a WeakHashMap to allow objects to be GC'd?

[scabug]

@DRMacIver said:
Don't be ridiculous. If something doesn't use something else, the compiler shouldn't insert a reference to it. That's a large part of the point of having lexical scoping. Code which relies on undocumented behaviour about garbage collecting outer classes is inherently broken, and code which puts classes in the place they logically belong shouldn't have to face a penalty because the compiler generates non-optimal code.

[scabug]

Oscar Boykin (oscar) said:
This issue hits scalding ( https://github.com/twitter/scalding ) because when we serialize inner classes on hadoop, they are pulling in references to the outer class, even if it is not used.

It makes generic serialization libraries (like Kryo, which we use) very difficult because we have to see if the outer$ parameter is really accessed or not (something Spark works around with ASM which is obviously an awful hack).

We see this issue on scala 2.9.2.

Our main work around now is to tell our users "don't do that". But it's difficult to explain to non-experts why it should matter.

[scabug]

@adriaanm said:
we hope to tackle this for 2.11 aka the performance edition,
it's too big a change for 2.10.X

[scabug]

@adriaanm said:
Unassigning and rescheduling to M6 as previous deadline was missed.

[scabug]

Daniel Vizzini (dvizzini) said (edited on Jul 3, 2014 5:44:39 PM UTC):
This is still an issue in Scalding ( https://github.com/twitter/scalding )

https://groups.google.com/forum/#!topic/cascading-user/-ChrHc3g1vo

It is responsible for numerous job failures, which often do not show up until a job has been run for more than an hour. Any work on this would greatly improve an increasingly popular open-source framework.

[scabug]

@nilskp said (edited on Jun 24, 2015 7:08:40 PM UTC):
Could someone comment on what makes this issue so hard to fix? It's killer for any kind of serialization scenario.

More specifically, is there any reason the outer instance is needed at runtime (if not referenced, of course)?

[scabug]

@adriaanm said (edited on Jun 24, 2015 7:17:44 PM UTC):
Separate compilation is one of the challenges, since you can't look at the class definition in order to decide whether we'll ever need to determine its outer instance. For example, the semantics of a type test pattern that includes a path, such as x: prefix.Nested compiles to x.outer eq prefix && x.isInstanceOf[Nested].

Is it an option to simply not nest this class?

PS: there's an undocumented "feature" that final nested classes don't receive outer pointers (unless required):

➜  ~  scalac -Xprint:cleanup /tmp/outer.scala
[[syntax trees at end of                   cleanup]] // outer.scala
package <empty> {
  class C extends Object {
    def <init>(): C = {
      C.super.<init>();
      ()
    }
  };
  final class C$FinalNested extends Object {
    def <init>($outer: C): C$FinalNested = {
      C$FinalNested.super.<init>();
      ()
    }
  };
  class C$Nested extends Object {
    <synthetic> <paramaccessor> <artifact> protected val $outer: C = _;
    <synthetic> <stable> <artifact> def $outer(): C = C$Nested.this.$outer;
    def <init>($outer: C): C$Nested = {
      if ($outer.eq(null))
        throw null
      else
        C$Nested.this.$outer = $outer;
      C$Nested.super.<init>();
      ()
    }
  }
}

for

class C {
  final class FinalNested 
  class Nested 
}

[scabug]

@adriaanm said:
To see the difference in pattern matching due to the lack of outer pointer, check out what the following snippet looks like at phase cleanup:

class D {
  val c1 = new C
  val c2 = new C
  val fn1 = new c1.FinalNested
  val fn2 = new c2.FinalNested
  val n1 = new c1.FinalNested
  val n2 = new c2.FinalNested

  (n1: Any) match { case x: c2.FinalNested => x}
  (fn1: Any) match { case x: c2.Nested => x}
}

[scabug]

@adriaanm said:
Spoiler:

- if (x1.$isInstanceOf[C$FinalNested]().&&(true))
+ if (x1.$isInstanceOf[C$Nested]().&&((x1.$asInstanceOf[C$Nested](): C$Nested).$outer().eq(D.this.c2())))

[scabug]

@nilskp said:
First of all, mind blown with throw null, hadn't seen that done before :-)

Although I'm happy to use final as a work-around, I'm not sure I fully understand why final semantics obviates the need for an outer instance. Is that because a non-final nested can be extended outside outer? Or what's the scenario here?

[scabug]

@adriaanm said:
When the nested class is final, you can reason locally about whether the outer accessor is needed (except for the pattern matching type test, which is weakened in the absence of the outer field). If the class is not final, a subclass (potentially defined in a separate compilation unit/run) may cause an outer accessor to be needed. See https://github.com/scala/scala/blob/2.11.x/src/compiler/scala/tools/nsc/transform/Constructors.scala#L113-L116

[scabug]

@nilskp said:
Thanks for the explanation, Adriaan!

I'm beginning to wish Scala's default would be final, with some keyword to allow extension, perhaps open class Foo. Maybe in Dotty? :-)

BTW, given the explanation, is there any hope this issue having any other resolution that "Won't Fix", i.e. why it's still open?

[scabug]

@adriaanm said:
Excellent question. A lot of tickets are open because we'd consider one day implementing them if we knew how to... I'm not a fan of that approach. I'd rather have a small set of open, actionable tickets, and close everything else until a repro/plan/owner emerges.

[scabug]

@adriaanm said (edited on Jun 26, 2015 12:28:13 AM UTC):
For this ticket, under a closed world assumption, the optimizer (such as the linker that Dmitry is working on) could infer finality much more aggressively.

[scabug]

@nilskp said:
So, if I understand this correctly, it's necessary for dealing with separate compilation units, which doesn't exist when using final. However, wouldn't that also apply to private nested classes? They also keep a reference to outer. They can be extended of course, but only in the same class thus compilation unit.

[scabug]

@adriaanm said:
Yes, good point -- private members are effectively final, which is a notion we introduced more recently than the potentially-elide-outers-for-final-classes optimization, I think, so it would make sense to move to the more general criterion.

[scabug]

@adriaanm said:
Looks like we already use the more general check, so private nested class should be treated like final ones. Is that not what you're seeing?

[scabug]

@adriaanm said (edited on Jun 26, 2015 3:27:06 PM UTC):
I just tried it myself -- a private class is indeed not optimized like a final one. I'll take a look. I started a discussion on isEffectivelyFinal: https://gitter.im/scala/scala?at=558d6e6c3e94a76222eaee76

[scabug]

@adriaanm said:
Here's a refinement. Tests pending. https://github.com/scala/scala/compare/2.12.x...adriaanm:refine-1419?expand=1

[scabug]

@nilskp said:
Also just realized that lifted nested methods contain outer reference:

  @Test
  def methodLift {
      def isPrime(c: Int) = BigInt(c).isProbablePrime(1)
    assertNoOuter(isPrime)
  }
  @Test
  def asFunction {
    val isPrime = (c: Int) => BigInt(c).isProbablePrime(1)
    assertNoOuter(isPrime)
  }
  private def assertNoOuter(f: Int => Boolean) {
    assertFalse(f.getClass.getDeclaredFields.exists(_.getType == this.getClass))
  }

methodLift fails, asFunction passes.

[scabug]

Oscar Boykin (oscar) said:
This last issue could have significant GC issues: you don't expect isPrime in methodLift to keep this in scope for the GC.

[scabug]

@nilskp said:
@moors, the last one I posted (lifted methods) is somewhat out of scope, per the definition of this ticket, but seems like a genuine fixable bug. Should I file it separately (or does it already exist)?

[scabug]

@nilskp said:
I opened a separate issue for the last one:
#9390

[scabug]

@adriaanm said:
Bumping since this should not require a binary incompatible change.

[scabug]

Oscar Boykin (oscar) said:
Adriaan, shouldn't it? There are classes that will have one fewer parameter in the constructor, no?

[scabug]

@adriaanm said:
You're right, thanks! Since this has an implementation, I'll see if I can still PR that refinement I did a year ago... We do have to ship RC1 at some point, so no promises :-)

[scabug]

@adriaanm said:
Aha! Even better, Jason already fixed this in #9408 (Avoid capturing outer class in local classes.)

[scabug]

@nilskp said:
Adriaan, is 9408 the same? It seems to be a change in capture, not an elimination.