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

Jump to bottom

Inlining can prolong object lifetimes leading to OutOfMemoryError #9137

Closed
scabug opened this issue Feb 6, 2015 · 10 comments · Fixed by scala/scala#7133
Closed

Inlining can prolong object lifetimes leading to OutOfMemoryError #9137

scabug opened this issue Feb 6, 2015 · 10 comments · Fixed by scala/scala#7133
Assignees
@lrytz
Labels
optimizer
Milestone
2.13.0-RC1

Comments

@scabug
Copy link

scabug commented Feb 6, 2015 

object Test {
  def main(args: Array[String]): Unit = {
    test()
  }
  def test() {
    new Stream().withFilter.next.next.next.next.next.next.next.next
  }
  class Stream() {
    println("new Stream()")
    val data = Array.ofDim[Byte](64 * 1024 * 1024)
    lazy val next = new Stream
    @inline
    final def withFilter = new StreamWithFilter(this)
  }

  final class StreamWithFilter(var s: Stream) {
    lazy val filtered = { val result = s.next; s = null; result}  
    def next: Stream = filtered.next
  }
}
% qscalac sandbox/test.scala && echo ':javap -c Test$#test' | qscala && qscala -nc Test
Welcome to Scala version 2.12.0-20150204-161502-f894c55ef3 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_25).
Type in expressions to have them evaluated.
Type :help for more information.

scala> :javap -c Test$#test
  public void test();
    Code:
       0: new           #22                 // class Test$Stream
       3: dup
       4: invokespecial #23                 // Method Test$Stream."<init>":()V
       7: invokevirtual #27                 // Method Test$Stream.withFilter:()LTest$StreamWithFilter;
      10: invokevirtual #33                 // Method Test$StreamWithFilter.next:()LTest$Stream;
      13: invokevirtual #34                 // Method Test$Stream.next:()LTest$Stream;
      16: invokevirtual #34                 // Method Test$Stream.next:()LTest$Stream;
      19: invokevirtual #34                 // Method Test$Stream.next:()LTest$Stream;
      22: invokevirtual #34                 // Method Test$Stream.next:()LTest$Stream;
      25: invokevirtual #34                 // Method Test$Stream.next:()LTest$Stream;
      28: invokevirtual #34                 // Method Test$Stream.next:()LTest$Stream;
      31: invokevirtual #34                 // Method Test$Stream.next:()LTest$Stream;
      34: pop
      35: return
}

scala> :quit
new Stream()
new Stream()
new Stream()
new Stream()
new Stream()
new Stream()
new Stream()
new Stream()
new Stream()
new Stream()

% qscalac -optimize -Yinline-warnings -Xfatal-warnings sandbox/test.scala && echo ':javap -c Test$#test' | qscala && qscala -nc Test
Welcome to Scala version 2.12.0-20150204-161502-f894c55ef3 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_25).
Type in expressions to have them evaluated.
Type :help for more information.

scala> :javap -c Test$#test
  public void test();
    Code:
       0: new           #22                 // class Test$Stream
       3: dup
       4: invokespecial #23                 // Method Test$Stream."<init>":()V
       7: astore_1
       8: new           #25                 // class Test$StreamWithFilter
      11: dup
      12: aload_1
      13: invokespecial #28                 // Method Test$StreamWithFilter."<init>":(LTest$Stream;)V
      16: invokevirtual #32                 // Method Test$StreamWithFilter.filtered:()LTest$Stream;
      19: invokevirtual #35                 // Method Test$Stream.next:()LTest$Stream;
      22: invokevirtual #35                 // Method Test$Stream.next:()LTest$Stream;
      25: invokevirtual #35                 // Method Test$Stream.next:()LTest$Stream;
      28: invokevirtual #35                 // Method Test$Stream.next:()LTest$Stream;
      31: invokevirtual #35                 // Method Test$Stream.next:()LTest$Stream;
      34: invokevirtual #35                 // Method Test$Stream.next:()LTest$Stream;
      37: invokevirtual #35                 // Method Test$Stream.next:()LTest$Stream;
      40: invokevirtual #35                 // Method Test$Stream.next:()LTest$Stream;
      43: pop
      44: return
}

scala> :quit
new Stream()
new Stream()
new Stream()
java.lang.OutOfMemoryError: Java heap space
    at scala.reflect.ManifestFactory$$anon$6.newArray(Manifest.scala:93)
    at scala.reflect.ManifestFactory$$anon$6.newArray(Manifest.scala:91)
@scabug
Copy link
Author

scabug commented Feb 6, 2015

Imported From: https://issues.scala-lang.org/browse/SI-9137?orig=1
Reporter: @retronym
Affected Versions: 2.11.5

@scabug
Copy link
Author

scabug commented Feb 6, 2015

@lrytz said:
one could argue that it's a good coincidence that the scala compiler doesn't introduce a local in the non-optimized case for the result of the withFilter call.. at least we don't have any specification of code generation that would ensure this.

for reference, adding links to Rex's suggestion of nulling out locals

@scabug
Copy link
Author

scabug commented Feb 6, 2015

@retronym said:
I'd say in absense of hard specs, the principle "do as in Java, unless it is unreasonable to do so" should apply to our code gen.

@scabug
Copy link
Author

scabug commented Feb 6, 2015

@retronym said:
In that spirit, it would be interesting to find out how inlining in hotspot interacts with GC.

@scabug scabug added this to the Backlog milestone Apr 7, 2017
@retronym retronym mentioned this issue May 2, 2017
Closed
@SethTisue
Copy link
Member

@lrytz should this stay open and assigned to you?

@lrytz
Copy link
Member

lrytz commented Mar 7, 2018

yes, keep it open

@lrytz lrytz mentioned this issue Aug 9, 2018
Merged
@lrytz lrytz mentioned this issue Aug 24, 2018
Merged
lrytz added a commit to lrytz/scala that referenced this issue Aug 25, 2018
@lrytz
Null out local vars created by the inliner
4cb2ef6 
This should fix scala/bug#9137
lrytz added a commit to lrytz/scala that referenced this issue Aug 25, 2018
@lrytz
Null out local vars created by the inliner
d384e38 
This should fix scala/bug#9137
lrytz added a commit to lrytz/scala that referenced this issue Aug 27, 2018
@lrytz
Null out local vars created by the inliner
2971c8d 
This should fix scala/bug#9137
@lrytz
Copy link
Member

lrytz commented Aug 27, 2018

There are two variants of this bug.

In the first one, the inliner creates a new local variable (in the example below for the receiver):

object Test {
  def main(args: Array[String]): Unit =
    new Stream().ident.next.next.next.next.next.next.next.next

  class Stream() {
    println("new Stream")

    val data = Array.ofDim[Byte](256 * 1024 * 1024)
    lazy val next = new Stream

    @noinline def id(s: Stream) = s
    @inline final def ident: Stream = id(this)
  }
}

This can be fixed by setting all local vars created by the inliner to null after the inlined code. However, the same problem exists for local variables copied into the callsite method:

object Test {
  def main(args: Array[String]): Unit =
    new Stream().ident.next.next.next.next.next.next.next.next

  class Stream() {
    println("new Stream")

    val data = Array.ofDim[Byte](256 * 1024 * 1024)
    lazy val next = new Stream

    @inline final def ident: Stream = {
      var alias: Stream = null
      if (data != null) alias = this
      alias
    }
  }
}

To avoid a full combined liveness&nullness analysis, we can just set all those local vars to null after inlining a method (part of scala/scala#7133).

lrytz added a commit to lrytz/scala that referenced this issue Aug 27, 2018
@lrytz
Null out local vars created by the inliner
a0e2ea4 
This should fix scala/bug#9137
@lrytz
Copy link
Member

lrytz commented Aug 27, 2018

For reference, someone implemented liveness analysis for nulling out locals using ASM, it seems under BSD license https://github.com/levans/Open-Quark/blob/master/src/CAL_Platform/src/org/openquark/cal/internal/javamodel/NullingClassAdapter.java

@Jasper-M
Copy link
Member

liveness analysis for nulling out locals

Not really related to inlining anymore, but something like this, either at bytecode or at Scala-language level, could also make the runtime semantics of block expressions more intuitive.

val x = {
  val tmp = getBigObject()
  tmp.mutate(42)
  processBigObject(tmp)
}
// tmp out of scope for Scala, but GC doesn't know it

@lrytz
Copy link
Member

lrytz commented Aug 27, 2018

Yes, I'm not saying it's a useless optimization. But at least the user can manually make tmp a var and null it out. If the issue is due to inlining, you're out of luck (except for @noinline). We'd also need to study if nulling out locals is universally a good idea (for performance), or if it can cause issues on its own.

So I think it makes sense to be eager about nulling out locals in the inliner for now.

lrytz added a commit to lrytz/scala that referenced this issue Aug 29, 2018
@lrytz
Null out local vars created by the inliner
6b4da82 
This should fix scala/bug#9137
lrytz added a commit to lrytz/scala that referenced this issue Sep 14, 2018
@lrytz
Null out local vars created by the inliner
b48e11b 
This should fix scala/bug#9137
lrytz added a commit to lrytz/scala that referenced this issue Sep 28, 2018
@lrytz
Null out local vars created by the inliner
8907558 
This should fix scala/bug#9137
@lrytz lrytz mentioned this issue Sep 28, 2018
Closed
lrytz added a commit to lrytz/scala that referenced this issue Sep 28, 2018
@lrytz
Null out local vars created by the inliner
8819c24 
This should fix scala/bug#9137
lrytz added a commit to lrytz/scala that referenced this issue Oct 11, 2018
@lrytz
Null out local vars created by the inliner
8f086f5 
This should fix scala/bug#9137
lrytz added a commit to lrytz/scala that referenced this issue Oct 11, 2018
@lrytz
Null out local vars created by the inliner
505b01c 
This should fix scala/bug#9137
@SethTisue SethTisue modified the milestones: Backlog, 2.13.0-RC1 Oct 26, 2018
NthPortal added a commit to NthPortal/scala that referenced this issue Dec 24, 2018
@NthPortal
Remove @noinline annotations from LazyList
449223f 
Remove `@noinline` annotations from `LazyList` since
scala/bug#9137 is fixed.
@NthPortal NthPortal mentioned this issue Dec 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lrytz lrytz

Labels
optimizer
Projects
None yet
Milestone
2.13.0-RC1
Development

Successfully merging a pull request may close this issue.

More capable inliner [ci: last-only] lrytz/scala
4 participants
@lrytz @SethTisue @Jasper-M @scabug