From: "Eregon (Benoit Daloze) via ruby-core" <ruby-core@...>
Date: 2023-02-13T12:52:11+00:00
Subject: [ruby-core:112402] [Ruby master Bug#19436] Call Cache for singleton methods can lead to "memory leaks"

Issue #19436 has been updated by 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".

----------------------------------------
Bug #19436: Call Cache for singleton methods can lead to "memory leaks"
https://bugs.ruby-lang.org/issues/19436#change-101842

* Author: byroot (Jean Boussier)
* Status: Open
* Priority: Normal
* Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN
----------------------------------------
Using "memory leaks" with quotes, because strictly speaking the memory isn't leaked, but it can nonetheless lead to large memory overheads.

### Minimal Reproduction

```ruby
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](https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-extending). 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:

```c
# 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 @ko1



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/