Skip to content

Use xfree in hash_st_free #9150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Use xfree in hash_st_free #9150

merged 1 commit into from
Dec 7, 2023

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Dec 7, 2023

st.c redefines malloc and free to be ruby_xmalloc and ruby_xfree, so when this was copied into hash.c it ended up mismatching an xmalloc with a regular free 😅, which ended up inflating oldmalloc_increase_bytes when hashes were freed by minor GC.

We found this by testing Ruby trunk in production, which showed a significant increase in major GC by cause of oldmalloc. After we were able to reproduce oldmalloc_increase_bytes increasing (semi-)consistently in a synthetic test. Finally I tracked down this location by enabling CALC_EXACT_MALLOC_SIZE and building with ASAN (which crashes consistently when trying to free an xmalloc).

Before:

$ ./miniruby -e '10.times { 1000.times { (0..10).map{[_1,_1]}.to_h }; GC.start(full_mark: false); puts GC.stat(:oldmalloc_increase_bytes) }'
169728
562752
946752
1330752
1714752
2098752
2482752
2866752
3250752
3634752

After

$ ./miniruby -e '10.times { 1000.times { (0..10).map{[_1,_1]}.to_h }; GC.start(full_mark: false); puts GC.stat(:oldmalloc_increase_bytes) }'
96
9120
9120
9120
9120
9120
9120
9120
9120
9120

st.c redefines malloc and free to be ruby_xmalloc and ruby_xfree, so
when this was copied into hash.c it ended up mismatching an xmalloc with
a regular free, which ended up inflating oldmalloc_increase_bytes when
hashes were freed by minor GC.
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.

Great find!

@jhawthorn jhawthorn merged commit 5f81f58 into ruby:master Dec 7, 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.

3 participants