Bug #20311
closed
Struct.new("A") memory leak?
Added by MaxLap (Maxime Lapointe) over 1 year ago. Updated over 1 year ago.
Description
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.
Updated by byroot (Jean Boussier) over 1 year ago Actions #1 [ruby-dev:52070]
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" 
Updated by nobu (Nobuyoshi Nakada) over 1 year ago Actions #2 [ruby-dev:52071]
Seems referred from defined_module_hash.
rb_const_remove needs to take care of it when the removed constant is a class or module?
Updated by byroot (Jean Boussier) over 1 year ago Actions #3 [ruby-dev:52072]
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.
Updated by byroot (Jean Boussier) over 1 year ago Actions #4 [ruby-dev:52073]
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.
Updated by byroot (Jean Boussier) over 1 year ago Actions #5 [ruby-dev:52074]
I think https://github.com/ruby/ruby/pull/10143 will do. But a good followup would also be to not pin Struct.new("A").
Updated by byroot (Jean Boussier) over 1 year ago Actions #6
- 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
Updated by byroot (Jean Boussier) over 1 year ago Actions #7 [ruby-dev:52075]
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
Updated by nobu (Nobuyoshi Nakada) over 1 year ago Actions #8 [ruby-dev:52077]
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?
Updated by nobu (Nobuyoshi Nakada) over 1 year ago Actions #9
- Description updated (diff)
Updated by nobu (Nobuyoshi Nakada) over 1 year ago Actions #10 [ruby-dev:52078]
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.
Updated by byroot (Jean Boussier) over 1 year ago Actions #11 [ruby-dev:52079]
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.
Updated by byroot (Jean Boussier) over 1 year ago Actions #12
- 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.
Updated by naruse (Yui NARUSE) over 1 year ago Actions #13 [ruby-dev:52081]
- 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.
Updated by nagachika (Tomoyuki Chikanaga) over 1 year ago Actions #14
- Backport changed from 3.0: WONTFIX, 3.1: REQUIRED, 3.2: REQUIRED, 3.3: DONE to 3.0: WONTFIX, 3.1: REQUIRED, 3.2: DONE, 3.3: DONE
ruby_3_2 bd5df1693c89d389471d145fc19b487c708912b1 merged revision(s) e626da82eae3d437b84d4f9ead0164d436b08e1a, f3af5ae7e6c1c096bbfe46d69de825a02b1696cf.