Bug #10537: Repeated creation and garbage collection of WeakRef instances against a single object leaks memory

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 2.2.0dev (2014-11-24 trunk 48552) [x86_64-darwin14]
• Backport: 2.0.0: REQUIRED, 2.1: REQUIRED

[javawizard (Alex Boyd)]

require 'weakref'
a = Object.new
1_000_000.times do
  WeakRef.new a
end
GC.start

The above results in Ruby consuming ~150 MB of RAM, all of which can only be freed by dropping a. This should not be the case - an object being weakly referenced should not itself hold a reference to the WeakRef (or any associated data) pointing at it.

[javawizard (Alex Boyd)]

Data point: tacking a printf onto the end of define_final0 in gc.c reveals that a new finalizer is being added to a for each WeakRef. I expect those are what's hogging memory.

[normalperson (Eric Wong)]

Thanks, the following patch should fix it. Your test runs much in less
than 20s and uses 10M on my older x86-64 machine. It took over 2
minutes before.

(lightly tested, and I'm unfamiliar with the weakmap code):

--- a/gc.c
+++ b/gc.c
@@ -2360,6 +2360,21 @@ define_final0(VALUE obj, VALUE block)
 
     if (st_lookup(finalizer_table, obj, &data)) {
 	table = (VALUE)data;
+
+	/* avoid duplicate block, table is usually small */
+	{
+	    const VALUE *ptr = RARRAY_CONST_PTR(table);
+	    long len = RARRAY_LEN(table);
+	    long i;
+
+	    for (i = 0; i < len; i++, ptr++) {
+		if (rb_funcall(*ptr, idEq, 1, block)) {
+		    rb_gc_force_recycle(block);
+		    return *ptr;
+		}
+	    }
+	}
+
 	rb_ary_push(table, block);
     }
     else {

[javawizard (Alex Boyd)]

Getting this:

irb(main):008:0> a = Object.new
=> #<Object:0x007fe1b983a278>
irb(main):009:0> WeakRef.new a
=> #<Object:0x007fe1b983a278>
irb(main):010:0> WeakRef.new a
NotImplementedError: method `==' called on broken T_???(0x10) object (0x007fe1b9842258 flags=0x7fe1b9842270)
	from /Users/aboyd/projects/ruby-trunk-install/lib/ruby/2.2.0/weakref.rb:87:in `[]='
	from /Users/aboyd/projects/ruby-trunk-install/lib/ruby/2.2.0/weakref.rb:87:in `initialize'
	from (irb):10:in `new'
	from (irb):10
	from bin/irb:11:in `<main>'

No time to dig in further right now, but I'll update when I do.

[normalperson (Eric Wong)]

Thanks for trying. I can not reproduce your broken object issue
with my patch on Linux (x86-64 and i686). "make check" passes, but
maybe something else is missing...

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

I can't reproduce the error, too.

What about this patch?

diff --git a/gc.c b/gc.c
index 9c0dbef..f4c4e93 100644
--- a/gc.c
+++ b/gc.c
@@ -7869,10 +7869,10 @@ wmap_aset(VALUE self, VALUE wmap, VALUE orig)
     TypedData_Get_Struct(self, struct weakmap, &weakmap_type, w);
     should_be_finalizable(orig);
     should_be_finalizable(wmap);
-    define_final0(orig, w->final);
-    define_final0(wmap, w->final);
-    st_update(w->obj2wmap, (st_data_t)orig, wmap_aset_update, wmap);
-    st_insert(w->wmap2obj, (st_data_t)wmap, (st_data_t)orig);
+    if (!st_update(w->obj2wmap, (st_data_t)orig, wmap_aset_update, wmap))
+	define_final0(orig, w->final);
+    if (!st_insert(w->wmap2obj, (st_data_t)wmap, (st_data_t)orig))
+	define_final0(wmap, w->final);
     return nonspecial_obj_id(orig);
 }
 

[normalperson (Eric Wong)]

nobu, your patch looks fine to me. However, my original uses less memory
and time on Alex's test on my Phenom II

[ruby-core:66430]
17.64user 0.01system 0:17.64elapsed 100%CPU (0avgtext+0avgdata 9544maxresident)k

[ruby-core:66457]
28.84user 0.01system 0:28.84elapsed 100%CPU (0avgtext+0avgdata 14172maxresident)k

Unfortunately, no word from Alex, yet...

On a related issue, Ctrl-C seems to be ignored with Alex's original
code. I'll have to dig through the threads/signal handling code again
to see what's wrong... (on Linux x86-64)

[javawizard (Alex Boyd)]

I'll see if I can get around to it tonight.

[javawizard (Alex Boyd)]

I'm no longer able to reproduce the issue on trunk, so this looks fine. I tested both patches and Eric's does indeed run faster and with less memory, with 7.3 MB and

real	0m5.028s
user	0m5.001s
sys 	0m0.024s

as opposed to ~20 MB and

real	0m18.736s
user	0m18.687s
sys 	0m0.041s

but both appear to work equally well other than that.

[normalperson (Eric Wong)]

nobu: any comment? Should I commit my original patch? Thanks.

We'll also need to investigate why Alex's test code does not appear
to handle signal delivery (Ctrl-C) in timely fashion...

• Status changed from Open to Closed
• % Done changed from 0 to 100

Applied in changeset r48820.

---

gc.c (define_final0): avoid duplicate blocks

This prevents excessive memory growth when a WeakRef
is repeatedly created

• gc.c (define_final0): avoid duplicate blocks
  [Bug #10537]
• test/test_weakref.rb (test_repeated_object_leak): new test

[normalperson (Eric Wong)]

nobu, your patch looks fine to me. However, my original uses less memory
and time on Alex's test on my Phenom II

[ruby-core:66430]
17.64user 0.01system 0:17.64elapsed 100%CPU (0avgtext+0avgdata 9544maxresident)k

[ruby-core:66457]
28.84user 0.01system 0:28.84elapsed 100%CPU (0avgtext+0avgdata 14172maxresident)k

Unfortunately, no word from Alex, yet...

On a related issue, Ctrl-C seems to be ignored with Alex's original
code. I'll have to dig through the threads/signal handling code again
to see what's wrong... (on Linux x86-64)

[nobu (Nobuyoshi Nakada)]

Yes, I confirmed the performance difference too.
I have no idea why it is slow, though.

[usa (Usaku NAKAMURA)]

• Backport changed from 2.0.0: UNKNOWN, 2.1: UNKNOWN to 2.0.0: REQUIRED, 2.1: REQUIRED

[usa (Usaku NAKAMURA)]

• Related to Bug #10618: TestWeakRef#test_repeated_object_leak fails on ARM added