Bug #10686: Memory leaking from torture test of symbol GC

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 2.2p0
• Backport: 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: DONE

[headius (Charles Nutter)]

The following code appears to grow without bounds when running on MRI 2.2p0 (and grows very fast...hold on to your RAM):

x = 0; loop { (x += 1).to_s.to_sym }

I asked ko1 about this on Twitter and he said it appears to be leaking strings somewhere.

[ko1 (Koichi Sasada)]

Confirming code:

require 'objspace'
require 'pp'

def sym_num; Symbol.all_symbols.size; end
x = 0
loop {
  (x += 1).to_s.to_sym
  if (x % 1000_000) == 0
    pp ObjectSpace.count_objects
  end
}

We can see that T_STRING is increasing.

[normalperson (Eric Wong)]

This seems to fix it:

--- a/symbol.c
+++ b/symbol.c
@@ -664,6 +664,7 @@ rb_gc_free_dsymbol(VALUE sym)
     if (str) {
 	RSYMBOL(sym)->fstr = 0;
 	unregister_sym(str, sym);
+	rb_hash_delete(global_symbols.dsymbol_fstr_hash, str);
     }
 }
 

http://80x24.org/spew/m/85a6346bf10d3f37fecb4926a57699fbd82b45b6.txt

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

Rather you may want call rb_hash_delete_entry().

[normalperson (Eric Wong)]

[e-mail] wrote:

Rather you may want call rb_hash_delete_entry().

Thanks, updated

http://80x24.org/spew/m/[e-mail]

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

Applied in changeset r49090.

---

symbol.c: fix memory leak from global fstr hash

• symbol.c (rb_gc_free_dsymbol): delete from global fstr hash
• test/ruby/test_symbol.rb (test_symbol_fstr_leak): test for bug
  [ruby-core:67268] [Bug #10686]

[nagachika (Tomoyuki Chikanaga)]

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

[naruse (Yui NARUSE)]

• Backport changed from 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: REQUIRED to 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: DONE

ruby_2_2 r49274 merged revision(s) 49090.

[swills (Steve Wills)]

I have to disagree that this isn't needed to be back ported to 2.1. I just tried it with my ruby 2.1 and saw memory growing rather quickly. My ruby -v:

ruby 2.1.5p273 (2014-11-13 revision 48405) [amd64-freebsd11]

[normalperson (Eric Wong)]

Symbol GC is a new feature in 2.2. In 2.1, symbols could never be GC-ed
at all so you'll see this growth, and I don't think it's the policy to
backport new features.

[swills (Steve Wills)]

Eric Wong wrote:

Symbol GC is a new feature in 2.2. In 2.1, symbols could never be GC-ed
at all so you'll see this growth, and I don't think it's the policy to
backport new features.

I see, thanks for the explanation. Not back porting makes sense then.

[ReiOdaira (Rei Odaira)]

• File test_symbol.patch test_symbol.patch added

I would like to propose two changes in test/ruby/test_symbol.rb (TestSymbol#test_symbol_fstr_leak).

First, this test checks only the increase in the virtual size, but it should check the rss as well. On Mac OS X, the memory leak of the symbols increases the rss, not the virtual size. The following example demonstrates the increase in the rss when the fix in symbol.c is not applied. Therefore, test_symbol_fstr_leak should specify "rss: true" as an argument to assert_no_memory_leak().

$ ruby --disable=gems -I test/lib -r memory_status -e 'p Memory::Status.new; 200_000.times { |i| i.to_s.to_sym }; GC.start; p Memory::Status.new'
#<struct Memory::Status size=2505801728, rss=4321280>
#<struct Memory::Status size=2563211264, rss=47579136>

The second change I am proposing is to add a warm-up phase before measuring the actual memory size increase. On a certain AIX machine, test_symbol_fstr_leak causes 2.6x increase in the virtual size, even after applying the fix in symbol.c.

$ ruby --disable=gems -I test/lib -r memory_status -e 'p Memory::Status.new; 200_000.times { |i| i.to_s.to_sym }; GC.start; p Memory::Status.new'
#<struct Memory::Status size=1646592, rss=4091904>
#<struct Memory::Status size=4263936, rss=6725632>

However, I don't think this is a leak, because if you sequentially run a similar test twice and only measure the second run, neither the virtual size or the rss increases so much, as follows.

$ ruby --disable=gems -I test/lib -r memory_status -e '200_000.times { |i| i.to_s.to_sym }; GC.start; p Memory::Status.new; 200_000.times { |i| (i+200_000).to_s.to_sym }; GC.start; p Memory::Status.new'
#<struct Memory::Status size=4276224, rss=6758400>
#<struct Memory::Status size=4321280, rss=6803456>

Because the warm-up phase inevitably increases memory usage, we should measure the steady state after the warm-up phase when detecting a memory leak.

The attached patch includes these two changes. I confirmed my patch on Mac OS X, POWER Linux, and AIX. Without the fix in symbol.c, test_symbol_fstr_leak correctly fails on every environment, and with the fix in symbol.c, it succeeds.

[nagachika (Tomoyuki Chikanaga)]

Applied at r52140.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: DONE to 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: REQUIRED

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: REQUIRED to 2.0.0: DONTNEED, 2.1: DONTNEED, 2.2: DONE

Backported r49813 and r52140 into ruby_2_2 branch at r52770.