Do inner classes cause a memory leak?

[Howard Lewis Ship]

On Fri, Jul 16, 2010 at 5:07 PM, David Holmes <David.Holmes at oracle.com> wrote:
> Howard,
>
> By definition a non-static inner class (nearly) always have a reference to
> an enclosing instance (modulo the bizarre edge-case of anonymous classes in
> static blocks).
>

See, that sounds to me like a language developer telling an
application developer how the world works. I'm often guilty on the
other side (as a framework developer telling application developers
what to do),
and I can tell you from personal experience that the adapting the
framework to their needs is often the right thing to do, even if it
means more work for the framework developer, but it ultimately results
in a better tool for everyone.

>From your point of view, there's no reason to define the inner class
inline, inside a instance method, if it doesn't need access to private
members of the containing instance.

As an application developer (and a framework author) there are times
when I want to keep related concepts close together in the code. Thus,
despite the bulky inner class syntax, I will often have a method
create and use or return an inner class. I don't like having to think
about this "memory leak" issue (I believe it's referred to elsewhere
as "shadowing").

It sounds like this is a recognized problem to be addressed as
stateless lambdas ... and that's fine.  However, making the compiler
smarter about inner classes (which are, after all, primarily syntactic
sugar for defining top-level classes, with some extra invisible
methods on the containing class to address visibility concerns) seems
like a win as well ... and in my estimation, it is possible to make
the compiler do the right thing without changing the semantics of the
generated code.  Again, via escape analysis the compiler should be
able to see if the inner class really does need access to the
surrounding instance and generate different code that does not pass
the containing object's this into the inner class' constructor.


> If there is truly no need to access an enclosing instance then don't use a
> non-static inner class.

Likewise, if I could say something like:

  new Thread(new static Runnable() { public void run() { ... }; });

... or some other notation, or even an annotation on the inner class,
that would be fine.   However simply saying "well that's the way the
compiler works and there's a workaround using a static method" is a
bit dismissive.  I think the whole spirit of lambda is to improve on
what Java is and does today, both in terms of syntax and semantics, to
keep it a vibrant and competitive language.  To be honest, I wasn't
tracking lambda very much until quite recently and I'm surprised at
how nicely the latest take on the syntax reads.

>
> David Holmes
>
> Howard Lewis Ship said the following on 07/17/10 03:28:
>>
>> I use a huge number of inner classes for closure-like operations in
>> Tapestry.  One thing I've noticed (often while using the debugger) is
>> that non-static inner classes appear to keep a reference to the
>> containing object, even when not absolutely necessary.
>>
>> Consider the following:
>>
>> package com.example;
>>
>> public class LeakExample
>> {
>>    public Runnable create()
>>    {
>>        return new Runnable()
>>        {
>>            public void run()
>>            {
>>                System.out.println("Inner class.");
>>            }
>>        };
>>    }
>> }
>>
>>
>> Usine JDK 1.6 on my Mac:
>>
>> $ jad -a -p  target/test-classes/com/example/LeakExample*.class
>> // Decompiled by Jad v1.5.8g. Copyright 2001 Pavel Kouznetsov.
>> // Jad home page: http://www.kpdus.com/jad.html
>> // Decompiler options: packimports(3) annotate
>> // Source File Name:   LeakExample.java
>>
>> package com.example;
>>
>> import java.io.PrintStream;
>>
>> public class LeakExample
>> {
>>
>>    public LeakExample()
>>    {
>>    //    0    0:aload_0
>>    //    1    1:invokespecial   #1   <Method void Object()>
>>    //    2    4:return
>>    }
>>
>>    public Runnable create()
>>    {
>>        return new Runnable() {
>>
>>            public void run()
>>            {
>>                System.out.println("Inner class.");
>>            //    0    0:getstatic       #3   <Field PrintStream
>> System.out>
>>            //    1    3:ldc1            #4   <String "Inner class.">
>>            //    2    5:invokevirtual   #5   <Method void
>> PrintStream.println(String)>
>>            //    3    8:return
>>            }
>>
>>            final LeakExample this$0;
>>
>>
>>            {
>>                this$0 = LeakExample.this;
>>            //    0    0:aload_0
>>            //    1    1:aload_1
>>            //    2    2:putfield        #1   <Field LeakExample this$0>
>>                super();
>>            //    3    5:aload_0
>>            //    4    6:invokespecial   #2   <Method void Object()>
>>            //    5    9:return
>>            }
>>        }
>> ;
>>    //    0    0:new             #2   <Class LeakExample$1>
>>    //    1    3:dup
>>    //    2    4:aload_0
>>    //    3    5:invokespecial   #3   <Method void
>> LeakExample$1(LeakExample)>
>>    //    4    8:areturn
>>    }
>> }
>> ~/work/t5-project/tapestry-ioc
>> $
>>
>>
>>
>>
>> ~/work/t5-project/tapestry-ioc
>> $ javac -version
>> javac 1.6.0_20
>>
>>
>> So looking at this, or the raw bytecode, it seems a non-static inner
>> class always includes a reference to the containing class, even when
>> (as in this example) the inner class has no dependencies on (private)
>> members of the enclosing class. Perhaps I'm misreading the dissembled
>> bytecode, but I think not.
>>
>> It seems to me that basic escape analysis should make it possible to
>> avoid this case, both for inner classes, and for the lambda syntax
>> being proposed. In fact, as long as the inner class only refers to
>> final instance variables (that could be passed to the constructed
>> inner class via its constructor) and not to methods of the outer
>> class, it should always be possible to avoid the leaked this
>> reference.
>>
>> This is important to me; Tapestry often follows a pattern wherein
>> large builder objects (full of mutable state) execute to produce final
>> runtime objects. Often inner classes (poor man's closures) are part of
>> this process. I want the builder objects to be GC'ed once the runtime
>> objects are created. It is all too easy for an inner class to hold a
>> reference to a builder object and prevent it from being GC'ed.
>>
>> Currently, I frequently resort to a static method to construct inner
>> classes so as to ensure that this does not escape (and with current
>> inner class syntax, that's often preferable from a code readability
>> point of view).  Further, when an inner class is used inline (with the
>> leaked this), my code only does so to gain access to a final field
>> (typically an injected dependency).
>>
>



-- 
Howard M. Lewis Ship

Creator of Apache Tapestry

The source for Tapestry training, mentoring and support. Contact me to
learn how I can get you up and productive in Tapestry fast!

(971) 678-5210
http://howardlewisship.com