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

yamam · 2018-01-12T12:52:45Z

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.

The text was updated successfully, but these errors were encountered:

kares · 2018-01-12T13:28:40Z

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

yamam · 2018-01-12T22:38:33Z

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 · 2018-01-25T18:08:13Z

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 · 2018-01-25T18:13:40Z

@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 · 2018-01-25T18:29:59Z

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

subbuss · 2018-01-25T18:34:40Z

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 · 2018-01-25T18:38:49Z

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 · 2018-01-25T20:34:50Z

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:

I think LVA is not tracking dead temps
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 · 2018-01-26T15:34:24Z

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 · 2018-01-26T19:07:10Z

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 · 2018-02-13T22:29:56Z

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.

headius · 2018-02-14T16:59:04Z

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 · 2018-02-14T17:13:26Z

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.

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 · 2018-02-14T17:17:59Z

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 · 2018-02-14T17:24:01Z

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 · 2018-02-14T17:26:35Z

Given that:

this only leaks one object reference per converted interface, and
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 · 2018-02-14T17:27:35Z

@headius yeah seems reasonable given the time scale.

kares · 2018-02-14T18:29:55Z

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 · 2018-02-17T03:35:11Z

@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 · 2018-04-02T09:44:07Z

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 · 2019-08-08T19:26:45Z

Ok, we've had a bunch of discussion on Matrix here:

https://matrix.to/#/!vyEDBdbmzqApWaugdd:matrix.org/$1565285938315885JSUJp:matrix.org?via=matrix.org

The summary of the problem goes like this:

When we take a proc and attach an interface impl to it, we make that proc into a singleton and then the singleton class connects to the interface impl. However the singleton holds a hard reference (via the "attached" field) to the proc, which in turn holds the binding. The Java calls to proc.call are made dynamically, which results in RuntimeCache (held in the Java interface impl class) holding a hard reference to the singleton class...which causes this leak.

This is an aborted patch attempting to use a real concrete class instead of a singleton class. It works for this bug, but breaks other interface impl paths due to the complexity of our existing logic (surrounding this "jcreate_meta" method).

https://gist.github.com/headius/f7316d00a541f0b44d66ddd2f71cb635

This is a simpler but perhaps questionable patch that just detaches the singleton class from the proc object. It also fixes the problem, but we are not sure what side effects result from having the "attached" object not actually be the proc. It seems like the attached object is mostly used for singleton hooks.

https://gist.github.com/headius/467a12f8cfc1ae2195060008185fbd0a

For 9.2.9 we will definitely not be trying to untangle the first patch, since there's a lot of unexplained logic from previous Java integration hackers. I will try pushing the second to a PR and see how it looks; I've confirmed it passes Java integration specs and the JRuby-specific test suite locally.

By holding a reference to the proc, we anchor the proc's binding to the singleton class. When methods from the singleton class are cached elsewhere, such as for Java interface proxies (see jrubyGH-4968) we end up keeping the proc's binding alive longer than we'd like. This patch detaches the proc object from the singleton, instead pointing it at the Proc class. The side effects from this include (at least) that any new singleton methods defined on that proc's singleton class will dispatch to hook methods (like singleton_method_defined) on Proc, rather than on the proc object. It's unknown whether this would affect any existing code.

This avoids detaching the object for user-provided singleton procs where they may actually want the hard reference and hook callbacks to work as before. See jrubyGH-4968.

This is a test that the fix for jrubyGH-4968 does not break existing singleton procs' "attached" relationship. The self in the singleton_method_added hook should still be the proc, rather than a reattached value as in the fix.

headius · 2019-08-08T20:48:32Z

Fixed by #5820. There's additional work possible here, to untangle the whole proc/interface mess, but for the purposes of this bug we no longer "leak" the binding.

By holding a reference to the proc, we anchor the proc's binding to the singleton class. When methods from the singleton class are cached elsewhere, such as for Java interface proxies (see jrubyGH-4968) we end up keeping the proc's binding alive longer than we'd like. This patch detaches the proc object from the singleton, instead pointing it at the Proc class. The side effects from this include (at least) that any new singleton methods defined on that proc's singleton class will dispatch to hook methods (like singleton_method_defined) on Proc, rather than on the proc object. It's unknown whether this would affect any existing code.

This avoids detaching the object for user-provided singleton procs where they may actually want the hard reference and hook callbacks to work as before. See jrubyGH-4968.

This is a test that the fix for jrubyGH-4968 does not break existing singleton procs' "attached" relationship. The self in the singleton_method_added hook should still be the proc, rather than a reattached value as in the fix.

Use of a resource collector with an override block will cause Puppet Server to retain `Puppet::Parser::Catalog` objects in memory after requests finish. The leak is caused by Resource Collector overriding the `child_of` methods on the resources it operates on to always return `true`. This forces the collector overrides to be merged in later by short-circuiting the logic in `Puppet::Parser::Resource` that only accepts overrides from a parent scope.[1][2] Using `meta_def` to override a method results in the creation of a Ruby "eigenclass" to hold the new method definition. The body of the method, which accepts `klass` and always returns `true`, is provided by Ruby block. This block is an instance of a `Proc`, which means that it closes over all variables that happen to be in scope. One of those variables is `@scope`, which is a reference to the compiler scope. JRuby doesn't always free eigenclass instances when the objects they were created to modify go out of scope.[3] Change the override logic to redefine the method using `instance_eval` instead of `meta_def`, which seems to produce the same effect but without leaving a reference to the `Proc` binding on the eigenclass. [1] https://github.com/puppetlabs/puppet/blob/6.18.0/lib/puppet/parser/resource.rb#L167-L169 [2] https://github.com/puppetlabs/puppet/blob/6.18.0/lib/puppet/parser/resource.rb#L361 [3] jruby/jruby#4968

Use of a resource collector with an override block will cause Puppet Server to retain `Puppet::Parser::Catalog` objects in memory after requests finish. The leak is caused by Resource Collector overriding the `child_of` methods on the resources it operates on to always return `true`. This forces the collector overrides to be merged in later by short-circuiting the logic in `Puppet::Parser::Resource` that only accepts overrides from a parent scope.[1][2] Using `meta_def` to override a method results in the creation of a Ruby "eigenclass" to hold the new method definition. The body of the method, which accepts `klass` and always returns `true`, is provided by a Ruby block. This block is an instance of a `Proc`, which means that it closes over all variables that happen to be in scope. One of those variables is `@scope`, which is a reference to the compiler scope. JRuby doesn't always free eigenclass instances when the objects they were created to modify go out of scope.[3] Change the override logic to redefine the method using `instance_eval` instead of `meta_def`, which seems to produce the same effect but without leaving a reference to the `Proc` binding on the eigenclass. [1] https://github.com/puppetlabs/puppet/blob/6.18.0/lib/puppet/parser/resource.rb#L167-L169 [2] https://github.com/puppetlabs/puppet/blob/6.18.0/lib/puppet/parser/resource.rb#L361 [3] jruby/jruby#4968

Use of a resource collector with an override block will cause Puppet Server to retain `Puppet::Parser::Catalog` objects in memory after requests finish. The leak is caused by Resource Collector overriding the `child_of` methods on the resources it operates on to always return `true`. This forces the collector overrides to be merged in later by short-circuiting the logic in `Puppet::Parser::Resource` that only accepts overrides from a parent scope.[1][2] Using `meta_def` to override a method results in the creation of a Ruby "eigenclass" to hold the new method definition. The body of the method, which accepts `klass` and always returns `true`, is provided by a Ruby block. This block is an instance of a `Proc`, which means that it closes over all variables that happen to be in scope. One of those variables is `@scope`, which is a reference to the compiler scope. JRuby doesn't always free eigenclass instances when the objects they were created to modify go out of scope.[3] Create an `override` accessor on the `Puppet::Resource::Type` class that we set on overrides, and update the `Puppet::Resource::Type#child_of?` method to check for that first. This way we completely avoid defining methods on an instance at runtime. [1] https://github.com/puppetlabs/puppet/blob/6.18.0/lib/puppet/parser/resource.rb#L167-L169 [2] https://github.com/puppetlabs/puppet/blob/6.18.0/lib/puppet/parser/resource.rb#L361 [3] jruby/jruby#4968

headius added the ir label Jan 25, 2018

headius mentioned this issue Feb 13, 2018

An object with defined singleton method and no external references is not GC from memory #4985

Closed

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

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

headius modified the milestones: JRuby 9.1.16.0, JRuby 9.1.17.0 Feb 14, 2018

headius mentioned this issue Feb 28, 2018

excessive loading of same InterfaceImpl #5023

Open

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

headius added this to the JRuby 9.2.2.0 milestone Oct 26, 2018

enebo modified the milestones: JRuby 9.2.2.0, JRuby 9.2.3.0, JRuby 9.2.4.0, JRuby 9.2.5.0 Nov 7, 2018

enebo modified the milestones: JRuby 9.2.5.0, JRuby 9.2.6.0 Dec 6, 2018

enebo modified the milestones: JRuby 9.2.6.0, JRuby 9.2.7.0 Dec 19, 2018

enebo modified the milestones: JRuby 9.2.7.0, JRuby 9.2.8.0 Jan 9, 2019

abrandoned mentioned this issue Jan 13, 2019

add benchmark-ips and remove the need to write a new method on an obj… liveh2o/protobuf-activerecord#39

Merged

headius mentioned this issue Aug 8, 2019

Dereference proc from interface impl singleton #5820

Merged

headius closed this as completed Aug 8, 2019

GabrielNagy mentioned this issue Sep 11, 2020

(PUP-10657) Fix resource collector override leak puppetlabs/puppet#8326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

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

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

yamam commented Jan 12, 2018

kares commented Jan 12, 2018

yamam commented Jan 12, 2018

headius commented Jan 25, 2018

headius commented Jan 25, 2018

headius commented Jan 25, 2018

subbuss commented Jan 25, 2018

subbuss commented Jan 25, 2018

enebo commented Jan 25, 2018 •

edited

Loading

subbuss commented Jan 26, 2018

enebo commented Jan 26, 2018

enebo commented Feb 13, 2018

headius commented Feb 14, 2018

headius commented Feb 14, 2018

headius commented Feb 14, 2018

headius commented Feb 14, 2018

headius commented Feb 14, 2018

enebo commented Feb 14, 2018

kares commented Feb 14, 2018

headius commented Feb 17, 2018

brometeo commented Apr 2, 2018

headius commented Aug 8, 2019 •

edited

Loading

headius commented Aug 8, 2019

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

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

Comments

yamam commented Jan 12, 2018

Environment

Expected Behavior

Actual Behavior

kares commented Jan 12, 2018

yamam commented Jan 12, 2018

headius commented Jan 25, 2018

headius commented Jan 25, 2018

headius commented Jan 25, 2018

subbuss commented Jan 25, 2018

subbuss commented Jan 25, 2018

enebo commented Jan 25, 2018 • edited Loading

subbuss commented Jan 26, 2018

enebo commented Jan 26, 2018

enebo commented Feb 13, 2018

headius commented Feb 14, 2018

headius commented Feb 14, 2018

headius commented Feb 14, 2018

headius commented Feb 14, 2018

headius commented Feb 14, 2018

enebo commented Feb 14, 2018

kares commented Feb 14, 2018

headius commented Feb 17, 2018

brometeo commented Apr 2, 2018

headius commented Aug 8, 2019 • edited Loading

headius commented Aug 8, 2019

enebo commented Jan 25, 2018 •

edited

Loading

headius commented Aug 8, 2019 •

edited

Loading