Bug #20311: Struct.new("A") memory leak?

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 3.3.0
• Backport: 3.0: WONTFIX, 3.1: REQUIRED, 3.2: REQUIRED, 3.3: DONE

[MaxLap (Maxime Lapointe)]

The following code gives the impression of a memory leak.

10.times do
  5000.times do
    Struct.new("A")
    Struct.send(:remove_const, :A)
  end

  GC.start
  puts `ps -o rss= -p #{$$}`.to_i
end

27868
35324
43400
51472
58676
66144
73764
81196
88512
95752

Is there another location where the struct gets set that I need to clear up for the GC free the memory?

Happens in 3.2.2, 3.2.3, 3.3.0, 3.3-head, ruby-head.

[byroot (Jean Boussier)]

I had a quick look at this using ObjectSpace.dump_all, and it appears that these classes are being retained in the VM root:

require 'objspace'

3.times do
  puts ObjectSpace.dump(Struct.new("A"))
  Struct.send(:remove_const, :A)
end

GC.start
ObjectSpace.dump_all(output: File.open("/tmp/heap.json", "w+"))

{"address":"0x102bc33f8", "type":"CLASS", ....
{"address":"0x102bc3218", "type":"CLASS", ....
{"address":"0x102bc3038", "type":"CLASS", ....

$ rg -F '"0x102bc3218"' /tmp/heap.json 
1:{"type":"ROOT", "root":"vm", "references":[..., "0x102bc3218"
$ rg -F '"0x102bc33f8"' /tmp/heap.json 
1:{"type":"ROOT", "root":"vm", "references":[..., "0x102bc33f8"

[nobu (Nobuyoshi Nakada)]

Seems referred from defined_module_hash.
rb_const_remove needs to take care of it when the removed constant is a class or module?

[byroot (Jean Boussier)]

rb_const_remove needs to take care of it when the removed constant is a class or module?

The problem is that it's a set:

st_insert(vm->defined_module_hash, (st_data_t)module, (st_data_t)module);

So if a module is added twice, you need to remove it from the set if it's removed twice... I'll see if I can do that.

But I also wonder why we have this root in the first place, I'd think these get marked from being constants in ::Object.

[byroot (Jean Boussier)]

But I also wonder why we have this root in the first place, I'd think these get marked from being constants in ::Object.

Nevermind, if I understand correctly, it's not to mark them, it's to pin them.

[byroot (Jean Boussier)]

I think https://github.com/ruby/ruby/pull/10143 will do. But a good followup would also be to not pin Struct.new("A").

[byroot (Jean Boussier)]

• Backport changed from 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN to 3.0: WONTFIX, 3.1: REQUIRED, 3.2: REQUIRED, 3.3: REQUIRED

[byroot (Jean Boussier)]

So https://github.com/ruby/ruby/pull/10143 is turning into a huge yak-shave and will be hard to backport. The reason is a lot of code end up relying on classes defined in C becoming immortal (in addition to being pinned).

So maybe a better short term solution that is easy to backport is to just not use these API for naming structs: https://github.com/ruby/ruby/pull/10144

[nobu (Nobuyoshi Nakada)]

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

So if a module is added twice, you need to remove it from the set if it's removed twice... I'll see if I can do that.

Is there any chance for the same module to be added twice?

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

[nobu (Nobuyoshi Nakada)]

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

So maybe a better short term solution that is easy to backport is to just not use these API for naming structs: https://github.com/ruby/ruby/pull/10144

Seems fine, but I'm afraid what can happen when an extension library stores rb_path2class("Struct::A") result then remove that constant?
Probably this is same for any Ruby-level defined classes/modules.

[byroot (Jean Boussier)]

Is there any chance for the same module to be added twice?

Yes, aliasing isn't uncommon.

Seems fine, but I'm afraid what can happen when an extension library stores rb_path2class("Struct::A") result then remove that constant?
Probably this is same for any Ruby-level defined classes/modules.

Yes, my reasoning is that it's not reasonable to expect classes created in Ruby to be immortal and immovable like the ones created in C.

[byroot (Jean Boussier)]

• Status changed from Open to Closed

Applied in changeset git|e626da82eae3d437b84d4f9ead0164d436b08e1a.

---

Don't pin named structs defined in Ruby

[Bug #20311]

rb_define_class_under assumes it's called from C and that the
reference might be held in a C global variable, so it adds the
class to the VM root.

In the case of Struct.new('Name') it's wasteful and make
the struct immortal.

[naruse (Yui NARUSE)]

• Backport changed from 3.0: WONTFIX, 3.1: REQUIRED, 3.2: REQUIRED, 3.3: REQUIRED to 3.0: WONTFIX, 3.1: REQUIRED, 3.2: REQUIRED, 3.3: DONE

ruby_3_3 f79b1d1ef1f7aa64d20f0eadbb3b0f8f7084deb3 merged revision(s) e626da82eae3d437b84d4f9ead0164d436b08e1a,f3af5ae7e6c1c096bbfe46d69de825a02b1696cf.