Bug #19436: Call Cache for singleton methods can lead to "memory leaks"

• Status: Open
• Priority: Normal
• Assignee: ko1 (Koichi Sasada)
• Target version: 3.3
• Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN

[byroot (Jean Boussier)]

Using "memory leaks" with quotes, because strictly speaking the memory isn't leaked, but it can nonetheless lead to large memory overheads.

Minimal Reproduction

module Foo
  def bar
  end
end

def call_bar(obj)
  # Here the call cache we'll keep a ref on the method_entry
  # which then keep a ref on the singleton_class, making that
  # instance immortal until the method is called again with
  # another instance.

  # The reference chain is IMEMO(callcache) -> IMEMO(ment) -> ICLASS -> CLASS(singleton) -> OBJECT
  obj.bar
end

obj = Object.new
obj.extend(Foo)

call_bar(obj)

id = obj.object_id
obj = nil

4.times { GC.start }
p ObjectSpace._id2ref(id)

Explanation

Call caches keep a strong reference onto the "callable method entry" (CME), which itself keeps a strong reference on the called object class
and in the cache of a singleton class, it keeps a strong reference onto the attached_object (instance).

This means that any call site that calls a singleton method, will effectively keep a strong reference onto the last receiver.
If the method is frequently called it's not too bad, but if it's infrequently called, it's effectively a (bounded) memory leak.
And if the attached_object is big, the wasted memory can be very substantial.

Practical Implications

Once relative common API impacted by this is Rails' extending API. This API allow to extend a "query result set" with a module.
These query results set can sometimes be very big, especially since they keep references to the instantiated ActiveRecord::Base instances etc.

Possible Solutions

Only keep a weak reference to the CME

The fairly "obvious" solution is to keep a weak reference to the CME, that's what I explored in https://github.com/ruby/ruby/pull/7272, and it seems to work.
However in debug mode It does fail on an assertion during compaction, but it's isn't quite clear to me what the impact is.

Additionally, something that makes me think this would be the right solution, is that call caches already try to avoid marking the class:

# vm_callinfo.h:275

struct rb_callcache {
    const VALUE flags;

    /* inline cache: key */
    const VALUE klass; // should not mark it because klass can not be free'd
                       // because of this marking. When klass is collected,
                       // cc will be cleared (cc->klass = 0) at vm_ccs_free().

So it appears that the class being also marked through the CME is some kind of oversight?

Don't cache based on some heuristics

If the above isn't possible or too complicated, an alternative would be to not cache CMEs found in singleton classes, except if it's the the singleton class of a Class or Module.

It would make repeated calls to such methods slower, but the assumption is that it's unlikely that these CME would live very long.

Make Class#attached_object a weak reference

Alternatively we could make the attached_object a weak reference, which would drastically limit the amount of memory that may be leaked in such scenario.

The downside is that Class#attached_object was very recently exposed in Ruby 3.2.0, so it means changing its semantic a bit.

cc @peterzhu2118 (Peter Zhu) @ko1 (Koichi Sasada)

[Eregon (Benoit Daloze)]

I think making Class#attached_object a weak reference is a better and easier solution.

Keeping a weak reference to the CME would actually be an unacceptable memory overhead on JVM, as it would mean one extra WeakReference object for each of these call caches.
I suppose there could be workarounds like storing the WeakReference on the singleton class, and use a direct reference otherwise, but then it'd make method lookup more complex and slower in interpreter or when polymorphic (need to handle cached method lookup on regular and singleton class differently).
FWIW not caching in calls to objects with a singleton class is not an option, it's important to cache for e.g. calling method on classes.
That approach is probably also quite difficult to get right on CRuby, also considering ractors and multithreading when the class GCs.

As a general note, creating a singleton class is not cheap, this should only be used for class objects (which always have one) and for a few rare global objects where it's convenient.
Using Object#extend objects often/on the fast path is just "making programs slow and uncached".

[byroot (Jean Boussier)]

FWIW not caching in calls to objects with a singleton class is not an option, it's important to cache for e.g. calling method on classes.

I specifically tried to say that the heuristic would be to not cache calls on singleton methods unless it's the singleton_class of a Module or Class.

Using Object#extend objects often/on the fast path is just "making programs slow and uncached".

Yeah I agree. I tried to avoid this in Rails by caching these dynamic classes, unfortunately that's not possible because of the API: https://github.com/rails/rails/pull/47314. So I'll try to see if this API can be replaced by a better one and deprecated.

[byroot (Jean Boussier)]

Keeping a weak reference to the CME would actually be an unacceptable memory overhead on JVM

Does that mean TruffleRuby suffer from the same "leak"? I was actually planning to ask you how it does deal with this case for inspiration.

[Eregon (Benoit Daloze)]

byroot (Jean Boussier) wrote in #note-2:

I specifically tried to say that the heuristic would be to not cache calls on singleton methods unless it's the singleton_class of a Module or Class.

I missed that sorry.
I think there are quite a few cases where a global object (which is not a module/class) with a singleton class is used, it'd be unfortunate and slow to not inline cache lookup in those cases.

I think it makes sense to only try caching one (or a few) singleton class(es), and give up when seeing a second one or more (already happens on TruffleRuby, but for a higher limit, 8 currently).
That means we still need to make Class#attached_object a weak reference.

[Eregon (Benoit Daloze)]

byroot (Jean Boussier) wrote in #note-3:

Does that mean TruffleRuby suffer from the same "leak"? I was actually planning to ask you how it does deal with this case for inspiration.

Yes, since TruffleRuby caches the the Class object in method lookup (LookupMethodNode), and currently the Class object has a strong reference to the attached_object.

[byroot (Jean Boussier)]

That means we still need to make Class#attached_object a weak reference.

I agree that conceptually, it's the simplest solution. That being said, in MRI weak references generally require a finalizer, which is annoying. But I'll see if I can implement that.

[ko1 (Koichi Sasada)]

Matz asked me to solve this memory leaking issue, so I'll consider about it before Ruby 3.3.
One idea is re-introduce class serial.

[byroot (Jean Boussier)]

One idea is re-introduce class serial.

Does that mean the method cache could be global again?

[Eregon (Benoit Daloze)]

@ko1 (Koichi Sasada) I think the real solution to this category of problems, not just this specific one is to make Class#attached_object (internally) a weak reference.
So I would strongly advise against changing the method lookup for this, it's an independent problem.
For instance, if someone does some caching in a Hash based on the Class, they don't expect holding onto a Class also holds onto the singleton object forever.

So Class#attached_object (the internal field used for it) is the leak, not the inline cache itself which needs to hold onto the class.

[Eregon (Benoit Daloze)]

Also as mentioned in https://bugs.ruby-lang.org/issues/19436#note-1 I believe TruffleRuby and JRuby (and probably more) would never use a weak reference in the inline cache, it's too much of a footprint overhead and also an extra indirection at least in the interpreter.
So then the only other solution I see is Class#attached_object (internally) being weak.
I think CRuby should follow the same idea, otherwise CRuby would be knowingly hurting compatibility between Ruby implementations here.

A per-class serial as far as I understand it is no good for a JIT, if e.g. methods are defined in some class late or repeatedly, it would invalidate all inline cache of method lookup on that class and all related JITed code, needlessly.
https://medium.com/graalvm/precise-method-and-constant-invalidation-in-truffleruby-4dd56c6bac1a has more details on that.

[ko1 (Koichi Sasada)]

byroot (Jean Boussier) wrote in #note-8:

One idea is re-introduce class serial.

Does that mean the method cache could be global again?

No. Current key is a class value (pointer) but the idea is making class serial (each class has a serial) as an inline cache key.

[ko1 (Koichi Sasada)]

Eregon (Benoit Daloze) wrote in #note-9:

@ko1 (Koichi Sasada) I think the real solution to this category of problems, not just this specific one is to make Class#attached_object (internally) a weak reference.

o = Object.new
c = o.singleton_class
o = nil

# do some task

p c.attached_object #=> ???

making weak reference, attached_objec may return nil for the collected object?

I'm not sure it is acceptable or not.

[Eregon (Benoit Daloze)]

ko1 (Koichi Sasada) wrote in #note-12:

making weak reference, attached_objec may return nil for the collected object?

I'm not sure it is acceptable or not.

Yes, this is indeed a consequence.
I think it's fine, Class#attached_object is very recent and this is necessary to solve a fundamental leak.

[Eregon (Benoit Daloze)]

ko1 (Koichi Sasada) wrote in #note-11:

No. Current key is a class value (pointer) but the idea is making class serial (each class has a serial) as an inline cache key.

That would mean an extra indirection/memory load on every cached method lookup, i.e. obj->klass == CACHED_CLASS (current) vs obj->klass->serial == CACHED_SERIAL.
Also it would require these serials to be unique, otherwise that's not a correct check (so maybe more of a "unique ID" than serial).

[ufuk (Ufuk Kayserilioglu)]

Let me add a few points in this conversation:

making weak reference, attached_objec may return nil for the collected object?

I'm not sure it is acceptable or not.

I agree with @Eregon (Benoit Daloze) on this point that this is totally acceptable. When I was proposing Class#attached_object the argument was that it would be helpful in cases where the objects are still supposed to be in memory for introspection purposes. There is very little value in trying to introspect an attached object that was already garbage collected, and it would not be great to retain objects just because they had a singleton class defined for them.

So, I am +1 for slightly changing the semantics of Class#attached_object before it becomes more widely used.

I should also say, though, that the purist in me still considers that the cache should be the one holding weak references, since, after all it is a cache and a miss should be relatively inconsequential, as opposed to the link between a singleton class and its attached object. I do understand, however, that it might not be the desired implementation and pragmatism over purism might be more warranted here.

[byroot (Jean Boussier)]

• Target version set to 3.3

[byroot (Jean Boussier)]

• Assignee set to ko1 (Koichi Sasada)