Skip to content

Commit

Permalink
Fix re-embedding of strings during compaction
Browse files Browse the repository at this point in the history
The reference updating code for strings is not re-embedding strings
because the code is incorrectly wrapped inside of a
`if (STR_SHARED_P(obj))` clause. Shared strings can't be re-embedded
so this ends up being a no-op. This means that strings can be moved to a
large size pool during compaction, but won't be re-embedded, which would
waste the space.
  • Loading branch information
peterzhu2118 committed Jan 9, 2023
1 parent 29dc937 commit 3be2acf
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 15 deletions.
16 changes: 9 additions & 7 deletions gc.c
Expand Up @@ -10609,16 +10609,18 @@ gc_update_object_references(rb_objspace_t *objspace, VALUE obj)
#if USE_RVARGC
VALUE new_root = any->as.string.as.heap.aux.shared;
rb_str_update_shared_ary(obj, old_root, new_root);
#endif
}

// if, after move the string is not embedded, and can fit in the
// slot it's been placed in, then re-embed it
if (rb_gc_obj_slot_size(obj) >= rb_str_size_as_embedded(obj)) {
if (!STR_EMBED_P(obj) && rb_str_reembeddable_p(obj)) {
rb_str_make_embedded(obj);
}
#if USE_RVARGC
/* If, after move the string is not embedded, and can fit in the
* slot it's been placed in, then re-embed it. */
if (rb_gc_obj_slot_size(obj) >= rb_str_size_as_embedded(obj)) {
if (!STR_EMBED_P(obj) && rb_str_reembeddable_p(obj)) {
rb_str_make_embedded(obj);
}
#endif
}
#endif

break;
}
Expand Down
12 changes: 8 additions & 4 deletions string.c
Expand Up @@ -312,14 +312,18 @@ rb_str_make_embedded(VALUE str)
RUBY_ASSERT(rb_str_reembeddable_p(str));
RUBY_ASSERT(!STR_EMBED_P(str));

char *buf = RSTRING_PTR(str);
long len = RSTRING_LEN(str);
char *buf = RSTRING(str)->as.heap.ptr;
long len = RSTRING(str)->as.heap.len;

STR_SET_EMBED(str);
STR_SET_EMBED_LEN(str, len);

memmove(RSTRING_PTR(str), buf, len);
ruby_xfree(buf);
if (len > 0) {
memcpy(RSTRING_PTR(str), buf, len);
ruby_xfree(buf);
}

TERM_FILL(RSTRING(str)->as.embed.ary + len, TERM_LEN(str));
}

void
Expand Down
8 changes: 4 additions & 4 deletions test/ruby/test_gc_compact.rb
Expand Up @@ -382,7 +382,7 @@ def add_ivars
def test_moving_strings_up_size_pools
omit if GC::INTERNAL_CONSTANTS[:SIZE_POOL_COUNT] == 1

assert_separately([], "#{<<~"begin;"}\n#{<<~"end;"}", timeout: 10, signal: :SEGV)
assert_separately(%w[-robjspace], "#{<<~"begin;"}\n#{<<~"end;"}", timeout: 10, signal: :SEGV)
begin;
STR_COUNT = 500
Expand All @@ -394,14 +394,14 @@ def test_moving_strings_up_size_pools
stats = GC.verify_compaction_references(expand_heap: true, toward: :empty)
assert_operator(stats[:moved_up][:T_STRING], :>=, STR_COUNT)
assert(ary) # warning: assigned but unused variable - ary
assert_include(ObjectSpace.dump(ary[0]), '"embedded":true')
end;
end

def test_moving_strings_down_size_pools
omit if GC::INTERNAL_CONSTANTS[:SIZE_POOL_COUNT] == 1

assert_separately([], "#{<<~"begin;"}\n#{<<~"end;"}", timeout: 10, signal: :SEGV)
assert_separately(%w[-robjspace], "#{<<~"begin;"}\n#{<<~"end;"}", timeout: 10, signal: :SEGV)
begin;
STR_COUNT = 500
Expand All @@ -412,7 +412,7 @@ def test_moving_strings_down_size_pools
stats = GC.verify_compaction_references(expand_heap: true, toward: :empty)
assert_operator(stats[:moved_down][:T_STRING], :>=, STR_COUNT)
assert(ary) # warning: assigned but unused variable - ary
assert_include(ObjectSpace.dump(ary[0]), '"embedded":true')
end;
end
end

0 comments on commit 3be2acf

Please sign in to comment.