Skip to content

Conversation

@HParker
Copy link
Contributor

@HParker HParker commented Sep 26, 2023

bug: https://bugs.ruby-lang.org/issues/19903

Reproduction:

10.times do
  100_000.times do
    RubyVM::InstructionSequence.load_from_binary(RubyVM::InstructionSequence.compile("def foo(bar:); end; foo(bar: :baz)").to_binary)
  end
  puts `ps -o rss= -p #{$$}`
end

before:

39636
60756
82140
103260
124380
145764
166884
188004
209388
230508

after:

25184
31784
38648
45248
51848
58712
65312
71912
78776
85376

Valgrind:

==62738== 496 bytes in 38 blocks are definitely lost in loss record 1 of 1
==62738==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==62738==    by 0x16F754: objspace_xmalloc0 (gc.c:12352)
==62738==    by 0x16FB5C: ruby_xmalloc2_body (gc.c:12599)
==62738==    by 0x173599: ruby_xmalloc2 (gc.c:14166)
==62738==    by 0x41D022: ibf_load_alloc (compile.c:11188)
==62738==    by 0x41E6BF: ibf_load_param_keyword (compile.c:11692)
==62738==    by 0x42098F: ibf_load_iseq_each (compile.c:12316)
==62738==    by 0x422A6F: rb_ibf_load_iseq_complete (compile.c:13183)
==62738==    by 0x422B59: ibf_load_iseq (compile.c:13238)
==62738==    by 0x41DD31: ibf_load_code (compile.c:11542)
==62738==    by 0x420AF5: ibf_load_iseq_each (compile.c:12326)
==62738==    by 0x422A6F: rb_ibf_load_iseq_complete (compile.c:13183)
==94377== 48 bytes in 1 blocks are definitely lost in loss record 1 of 1
==94377==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==94377==    by 0x16F754: objspace_xmalloc0 (gc.c:12352)
==94377==    by 0x16FB5C: ruby_xmalloc2_body (gc.c:12599)
==94377==    by 0x173599: ruby_xmalloc2 (gc.c:14166)
==94377==    by 0x3EFC5A: ISEQ_ORIGINAL_ISEQ_ALLOC (iseq.h:77)
==94377==    by 0x3F6B50: rb_iseq_original_iseq (compile.c:923)
==94377==    by 0x41D6C7: ibf_dump_code (compile.c:11404)
==94377==    by 0x41F7DF: ibf_dump_iseq_each (compile.c:12027)
==94377==    by 0x420C3E: ibf_dump_iseq_list_i (compile.c:12361)
==94377==    by 0x2BDEF8: apply_functor (st.c:1617)
==94377==    by 0x2BDB5B: st_general_foreach (st.c:1527)
==94377==    by 0x2BDF56: rb_st_foreach (st.c:1624)

This improves memory handling and reduces memory growth over time.
I didn't add a leak test because I can't reproduce this issue before the test times out.

@HParker HParker force-pushed the fix-iseq-memory-leaks branch from 92dc74c to 9dab5bc Compare September 26, 2023 13:50
Copy link
Member

@peterzhu2118 peterzhu2118 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, otherwise looks good!

iseq.c Outdated
Comment on lines 191 to 192
if (body->variable.original_iseq != NULL)
ruby_xfree(body->variable.original_iseq);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ruby_xfree can handle NULL values, so we don't need to explicitly check that here.

Suggested change
if (body->variable.original_iseq != NULL)
ruby_xfree(body->variable.original_iseq);
ruby_xfree(body->variable.original_iseq);

[bug #19903]

Co-authored-by: Peter Zhu <peter@peterzhu.ca>
@HParker HParker force-pushed the fix-iseq-memory-leaks branch from 9dab5bc to 499d9c5 Compare September 26, 2023 14:02
@peterzhu2118 peterzhu2118 merged commit ef59175 into ruby:master Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants