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

proc binding(local variables) is not garbage collected (memory leak) #4968

Open
yamam opened this Issue Jan 12, 2018 · 23 comments

Comments

Projects
None yet
6 participants
@yamam

yamam commented Jan 12, 2018

Local variables which is binded by a proc is not garbage collected, if the proc is passed to java function.

Environment

jruby 9.1.0.0 (2.3.0) 2016-05-02 a633c63 Java HotSpot(TM) 64-Bit Server VM 25.151-b12 on 1.8.0_151-b12 +jit [mswin32-x86_64]

Expected Behavior

JavaClass.java

public class JavaClass
{
    public JavaClass(Runnable proc)
    {
        proc.run();
    }
}

memoryleak.rb

import 'JavaClass'

class BigClass
    # 100MB class
    def initialize
        @a = "a" * (100 * 1024 ** 2)
    end
end

def func
    a = BigClass.new
    JavaClass.new do
        puts "callback"
    end
end

func

# GC by Java VisualVM

gets

$ javac JavaClass.java
$ jruby-9.0.5.0/bin/jruby memoryleak.rb

run GC by jvisualvm

after running GC, memory usage is reduced about 100MB.

Actual Behavior

$ javac JavaClass.java
$ jruby-9.1.0.0/bin/jruby memoryleak.rb (9.1.0.0 or above)

run GC by jvisualvm

after running GC, memory usage is not reduced.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jan 12, 2018

Member

just try the latest off JRuby 9.1 and you should be good to do, recall this has been fixed along the way

Member

kares commented Jan 12, 2018

just try the latest off JRuby 9.1 and you should be good to do, recall this has been fixed along the way

@yamam

This comment has been minimized.

Show comment
Hide comment
@yamam

yamam Jan 12, 2018

I tried jruby 9.1.15 and 9.1.16-SNAPSHOT. The result is the same as 9.1.0.0.
I also tried jruby 9.2.0.0-SNAPSHOT, but another error occured, and the script couldn't run

jruby-9.2.0.0-SNAPSHOT/bin/jruby memoryleak.rb
unknown encoding name - MS932

yamam commented Jan 12, 2018

I tried jruby 9.1.15 and 9.1.16-SNAPSHOT. The result is the same as 9.1.0.0.
I also tried jruby 9.2.0.0-SNAPSHOT, but another error occured, and the script couldn't run

jruby-9.2.0.0-SNAPSHOT/bin/jruby memoryleak.rb
unknown encoding name - MS932
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 25, 2018

Member

This is likely because the result of func gets stored into a temporary local variable but never used or cleared. This causes the JavaClass object, the block and binding it captures, and the BigClass stored in that binding to be retained.

The IR for the top-level method, from the func call down:

  10:  %v_7 := call_0o(self<%self>, callType: VARIABLE, name: func, potentiallyRefined: false)
  11:          line_num(lineNumber: 20)
  12:  %v_8 := call_0o(self<%self>, callType: VARIABLE, name: gets, potentiallyRefined: false)
  13:          pop_binding
  14:          pop_method_frame
  15:          return(%v_8)

That %v_7 goes into a JVM local variable, and the JVM considers it alive for purposes of GC.

Member

headius commented Jan 25, 2018

This is likely because the result of func gets stored into a temporary local variable but never used or cleared. This causes the JavaClass object, the block and binding it captures, and the BigClass stored in that binding to be retained.

The IR for the top-level method, from the func call down:

  10:  %v_7 := call_0o(self<%self>, callType: VARIABLE, name: func, potentiallyRefined: false)
  11:          line_num(lineNumber: 20)
  12:  %v_8 := call_0o(self<%self>, callType: VARIABLE, name: gets, potentiallyRefined: false)
  13:          pop_binding
  14:          pop_method_frame
  15:          return(%v_8)

That %v_7 goes into a JVM local variable, and the JVM considers it alive for purposes of GC.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 25, 2018

Member

@enebo @subbuss For this particular case, we could modify the compiler to not store a result, yes? The old JIT did this by passing an "expression" flag through the AST that indicated whether the result would be used or not.

A related question is whether the assignment could be removed by a smarter DCE?

Member

headius commented Jan 25, 2018

@enebo @subbuss For this particular case, we could modify the compiler to not store a result, yes? The old JIT did this by passing an "expression" flag through the AST that indicated whether the result would be used or not.

A related question is whether the assignment could be removed by a smarter DCE?

@headius headius added the ir label Jan 25, 2018

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 25, 2018

Member

A simple workaround is to do the func call within another scope (without it returning a result).

Member

headius commented Jan 25, 2018

A simple workaround is to do the func call within another scope (without it returning a result).

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Jan 25, 2018

Contributor

A related question is whether the assignment could be removed by a smarter DCE?

Possibly ... I think that information is already present in the DCE pass and could be used to kill the store.

Contributor

subbuss commented Jan 25, 2018

A related question is whether the assignment could be removed by a smarter DCE?

Possibly ... I think that information is already present in the DCE pass and could be used to kill the store.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Jan 25, 2018

Contributor

Method markDeadInstructions could mark result vars that are dead after that definition which could then be used by JIT / interpreter. I thought I had this information already marked in the results at one point. But, that is where you should look.

Contributor

subbuss commented Jan 25, 2018

Method markDeadInstructions could mark result vars that are dead after that definition which could then be used by JIT / interpreter. I thought I had this information already marked in the results at one point. But, that is where you should look.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jan 25, 2018

Member

I sort of remember something where LVA was not tracking anything but LocalVariables? I do not know why we never included Temporary Variables but it seems like we should be able to. So I think there are two possible issues:

  1. I think LVA is not tracking dead temps
  2. There appears to be no discardResult callers anymore?

But seemingly I think all the pieces are more or less already there we just need to do some modifications to hook them back up.

This does only reduce the issue we have struggled with for a few years that register design pins values in temps. Had this been %v_8 and we kept this frame we would still leak.

Member

enebo commented Jan 25, 2018

I sort of remember something where LVA was not tracking anything but LocalVariables? I do not know why we never included Temporary Variables but it seems like we should be able to. So I think there are two possible issues:

  1. I think LVA is not tracking dead temps
  2. There appears to be no discardResult callers anymore?

But seemingly I think all the pieces are more or less already there we just need to do some modifications to hook them back up.

This does only reduce the issue we have struggled with for a few years that register design pins values in temps. Had this been %v_8 and we kept this frame we would still leak.

@subbuss

This comment has been minimized.

Show comment
Hide comment
@subbuss

subbuss Jan 26, 2018

Contributor

Historical reasons why LVA didn't track temps. But, yes, LVA can be readily extended to track all vars, including temps. And. dead results should be discarded in interp / jit.

Contributor

subbuss commented Jan 26, 2018

Historical reasons why LVA didn't track temps. But, yes, LVA can be readily extended to track all vars, including temps. And. dead results should be discarded in interp / jit.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jan 26, 2018

Member

Recalling another conversation if we continue to see insurmountable cases we may need to nil out temps at the point they are no longer used. That will be no fun for interp but I doubt will hurt JIT. We could possibly even optimize that if we know an lvar still captures same reference and not both to emit the nilling out.

Member

enebo commented Jan 26, 2018

Recalling another conversation if we continue to see insurmountable cases we may need to nil out temps at the point they are no longer used. That will be no fun for interp but I doubt will hurt JIT. We could possibly even optimize that if we know an lvar still captures same reference and not both to emit the nilling out.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Feb 13, 2018

Member

Alright I looked at this a bit this afternoon. I do not think this is an issue with IR (although all things mentioned above are no doubt likely in our IR impl today). The original reporter noticed this happens as part of passing this to Java. If I eliminate java from the equation this immediately cleans up the memory held by the big instance variable. I also see nothing specifically capturing the block which captures the ivar in IR.

My current theory: When we construct a Runnable out of the proc and something continues to reference that. @kares any chance you can relook at this? I tried looking around but I really don't follow JI very much anymore. I thought obj.toJava() was happening for the RubyProc but nada. I am missing something.

I am going to mark against 9.1.16.0 since I am not sure of the full ramifications of when we keep a reference to procs converted to other types.

Member

enebo commented Feb 13, 2018

Alright I looked at this a bit this afternoon. I do not think this is an issue with IR (although all things mentioned above are no doubt likely in our IR impl today). The original reporter noticed this happens as part of passing this to Java. If I eliminate java from the equation this immediately cleans up the memory held by the big instance variable. I also see nothing specifically capturing the block which captures the ivar in IR.

My current theory: When we construct a Runnable out of the proc and something continues to reference that. @kares any chance you can relook at this? I tried looking around but I really don't follow JI very much anymore. I thought obj.toJava() was happening for the RubyProc but nada. I am missing something.

I am going to mark against 9.1.16.0 since I am not sure of the full ramifications of when we keep a reference to procs converted to other types.

@enebo enebo added this to the JRuby 9.1.16.0 milestone Feb 13, 2018

@enebo enebo added java integration and removed ir labels Feb 13, 2018

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2018

Member

Ok, I was half right and @enebo was half right.

The GC root holding on to this thing is the IR variable. But the real bug is why the binding for the JavaClass.new block is still alive after that call completes. We are investigating.

Member

headius commented Feb 14, 2018

Ok, I was half right and @enebo was half right.

The GC root holding on to this thing is the IR variable. But the real bug is why the binding for the JavaClass.new block is still alive after that call completes. We are investigating.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2018

Member

I think I see the issue.

For the new interface impl, we generated a new InterfaceImpl class. This class has a static field holding a RuntimeCache object that's used to cache calls via the interface into the associated Ruby object. So thatis part of it...the call cache holds a reference to the proc object, anchoring the binding along with it.

However the InterfaceImpl class should be unrooted and go away once it's not needed.

image

This is a graph from the InterfaceImpl object and class to the neared GC root. It appears that the interface impl is being generated into a JRubyClassLoader that's held by our Ruby object, rather than into its own unrooted classloader. We've made most code generation go into unrooted classloaders other places, but it seems we may not have done that here.

Member

headius commented Feb 14, 2018

I think I see the issue.

For the new interface impl, we generated a new InterfaceImpl class. This class has a static field holding a RuntimeCache object that's used to cache calls via the interface into the associated Ruby object. So thatis part of it...the call cache holds a reference to the proc object, anchoring the binding along with it.

However the InterfaceImpl class should be unrooted and go away once it's not needed.

image

This is a graph from the InterfaceImpl object and class to the neared GC root. It appears that the interface impl is being generated into a JRubyClassLoader that's held by our Ruby object, rather than into its own unrooted classloader. We've made most code generation go into unrooted classloaders other places, but it seems we may not have done that here.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2018

Member

Well there's yer problem...

        // if it's a singleton class and the real class is proc, we're doing closure conversion
        // so just use Proc's hashcode
        if ( isProc ) {
            interfacesHashCode = 31 * interfacesHashCode + runtime.getProc().hashCode();
            classLoader = jrubyClassLoader;
        }
        else { // normal new class implementing interfaces
            interfacesHashCode = 31 * interfacesHashCode + wrapperClass.getRealClass().hashCode();
            classLoader = new OneShotClassLoader(jrubyClassLoader);
        }

Because this is a proc, we're using the "natural" JRubyClassLoader, which is held by the Ruby instance. Due to the caches this new class contains, that holds the proc, which holds the binding, which holds the big data.

Member

headius commented Feb 14, 2018

Well there's yer problem...

        // if it's a singleton class and the real class is proc, we're doing closure conversion
        // so just use Proc's hashcode
        if ( isProc ) {
            interfacesHashCode = 31 * interfacesHashCode + runtime.getProc().hashCode();
            classLoader = jrubyClassLoader;
        }
        else { // normal new class implementing interfaces
            interfacesHashCode = 31 * interfacesHashCode + wrapperClass.getRealClass().hashCode();
            classLoader = new OneShotClassLoader(jrubyClassLoader);
        }

Because this is a proc, we're using the "natural" JRubyClassLoader, which is held by the Ruby instance. Due to the caches this new class contains, that holds the proc, which holds the binding, which holds the big data.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2018

Member

So the immediate fix is that we should be generating both paths into a "OneShotClassLoader" so it can be unrooted and GC properly.

The down side is that the logic is is intentional. It's designed to only generate the interface-to-proc mapping once per interface, avoiding spinning new classes for what is essentially the same thing every time. That's admirable, but with the hidden state on these classes, we end up holding a reference to whatever the last proc was used by this interface impl, and there's where this problem comes in.

So it is a "leak", but it's only a leak of one target proc at a time.

In order to fix this properly (i.e. without introducing a ton of overhead and wasted classes+classloaders), we need to move the method cache into an instance field of the generated InterfaceImpl, so it's only per-object that it holds these references. Then when it goes away, whatever it cached is dereferenced.

This might be possible to do for 9.1.16, but it's scary to mess with this days before release.

Long term, this interface impl generation needs a complete retooling to avoid all this static state and be more reusable across procs and interfaces.

Member

headius commented Feb 14, 2018

So the immediate fix is that we should be generating both paths into a "OneShotClassLoader" so it can be unrooted and GC properly.

The down side is that the logic is is intentional. It's designed to only generate the interface-to-proc mapping once per interface, avoiding spinning new classes for what is essentially the same thing every time. That's admirable, but with the hidden state on these classes, we end up holding a reference to whatever the last proc was used by this interface impl, and there's where this problem comes in.

So it is a "leak", but it's only a leak of one target proc at a time.

In order to fix this properly (i.e. without introducing a ton of overhead and wasted classes+classloaders), we need to move the method cache into an instance field of the generated InterfaceImpl, so it's only per-object that it holds these references. Then when it goes away, whatever it cached is dereferenced.

This might be possible to do for 9.1.16, but it's scary to mess with this days before release.

Long term, this interface impl generation needs a complete retooling to avoid all this static state and be more reusable across procs and interfaces.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 14, 2018

Member

Given that:

  1. this only leaks one object reference per converted interface, and
  2. the safe fix for 9.1.16.0 will introduce lots of overhead into proc conversion

I vote that we do not fix this for 9.1.16.0 and move forward with a "proper" fix I describe above in 9.1.17.0 (or 9.2, whichever comes first).

Member

headius commented Feb 14, 2018

Given that:

  1. this only leaks one object reference per converted interface, and
  2. the safe fix for 9.1.16.0 will introduce lots of overhead into proc conversion

I vote that we do not fix this for 9.1.16.0 and move forward with a "proper" fix I describe above in 9.1.17.0 (or 9.2, whichever comes first).

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Feb 14, 2018

Member

@headius yeah seems reasonable given the time scale.

Member

enebo commented Feb 14, 2018

@headius yeah seems reasonable given the time scale.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Feb 14, 2018

Member

great find, not sure but maybe this should 'not' get fixed really - generating a new class every time.
... there's already another bug against not caching a generated class, in some other case related to JI

Member

kares commented Feb 14, 2018

great find, not sure but maybe this should 'not' get fixed really - generating a new class every time.
... there's already another bug against not caching a generated class, in some other case related to JI

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 17, 2018

Member

@kares I assume you mean #5023? That case is partially bad usage of our interface impl logic; it's essentially forcing the system to generate a new impl every time. I suggested the proper way to do this simple interface implementation which would not have the same problem.

But your point stands...we really ought to be able to have a single class in memory for each grouped set of interfaces, dispatching exactly like it does currently but with a "less static" inline caching mechanism.

Member

headius commented Feb 17, 2018

@kares I assume you mean #5023? That case is partially bad usage of our interface impl logic; it's essentially forcing the system to generate a new impl every time. I suggested the proper way to do this simple interface implementation which would not have the same problem.

But your point stands...we really ought to be able to have a single class in memory for each grouped set of interfaces, dispatching exactly like it does currently but with a "less static" inline caching mechanism.

@brometeo

This comment has been minimized.

Show comment
Hide comment
@brometeo

brometeo Apr 2, 2018

This bug seems marked for 9.1.17.0, but not for 9.2.0.0. What is the current development state?
Thank you.

brometeo commented Apr 2, 2018

This bug seems marked for 9.1.17.0, but not for 9.2.0.0. What is the current development state?
Thank you.

@headius headius modified the milestones: JRuby 9.1.17.0, JRuby 9.2.0.0 Apr 11, 2018

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 11, 2018

Member

The more elaborate fix will not make 9.1.17.0 since we want to flip that fairly quickly. We hope to release 9.2 before the end of May.

Member

headius commented Apr 11, 2018

The more elaborate fix will not make 9.1.17.0 since we want to flip that fairly quickly. We hope to release 9.2 before the end of May.

@brometeo

This comment has been minimized.

Show comment
Hide comment
@brometeo

brometeo Apr 11, 2018

And the fix will go in 9.2?

brometeo commented Apr 11, 2018

And the fix will go in 9.2?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Apr 12, 2018

Member

@brometeo I sure hope so :-)

Member

headius commented Apr 12, 2018

@brometeo I sure hope so :-)

@enebo enebo modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 24, 2018

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