Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
scabug opened this issue Oct 11, 2008 · 35 comments
Closed

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

scabug opened this issue Oct 11, 2008 · 35 comments

Comments

@scabug
Copy link

scabug commented Oct 11, 2008

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
Copy link
Author

scabug commented Oct 11, 2008

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

@scabug
Copy link
Author

scabug commented Oct 13, 2008

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

@scabug
Copy link
Author

scabug commented Oct 13, 2008

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

@scabug
Copy link
Author

scabug commented Oct 13, 2008

@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
Copy link
Author

scabug commented Oct 13, 2008

@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
Copy link
Author

scabug commented Oct 13, 2008

@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
Copy link
Author

scabug commented Oct 13, 2008

@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
Copy link
Author

scabug commented Oct 13, 2008

@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
Copy link
Author

scabug commented Jul 19, 2012

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
Copy link
Author

scabug commented Aug 4, 2012

@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
Copy link
Author

scabug commented Jul 10, 2013

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

@scabug
Copy link
Author

scabug commented Jul 3, 2014

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
Copy link
Author

scabug commented Jun 24, 2015

@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
Copy link
Author

scabug commented Jun 24, 2015

@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
Copy link
Author

scabug commented Jun 24, 2015

@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
Copy link
Author

scabug commented Jun 24, 2015

@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
Copy link
Author

scabug commented Jun 25, 2015

@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
Copy link
Author

scabug commented Jun 25, 2015

@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
Copy link
Author

scabug commented Jun 25, 2015

@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
Copy link
Author

scabug commented Jun 26, 2015

@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
Copy link
Author

scabug commented Jun 26, 2015

@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
Copy link
Author

scabug commented Jun 26, 2015

@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
Copy link
Author

scabug commented Jun 26, 2015

@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
Copy link
Author

scabug commented Jun 26, 2015

@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
Copy link
Author

scabug commented Jun 26, 2015

@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
Copy link
Author

scabug commented Jun 26, 2015

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

@scabug
Copy link
Author

scabug commented Jun 27, 2015

@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
Copy link
Author

scabug commented Jun 29, 2015

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
Copy link
Author

scabug commented Jul 2, 2015

@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
Copy link
Author

scabug commented Jul 10, 2015

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

@scabug
Copy link
Author

scabug commented Jul 26, 2016

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

@scabug
Copy link
Author

scabug commented Jul 27, 2016

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

@scabug
Copy link
Author

scabug commented Aug 2, 2016

@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
Copy link
Author

scabug commented Aug 2, 2016

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

@scabug scabug closed this as completed Aug 2, 2016
@scabug
Copy link
Author

scabug commented Aug 2, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants