Skip to content
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

Handle running out of shapes in Object#dup #8799

Merged
merged 5 commits into from Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion object.c
Expand Up @@ -326,9 +326,18 @@ rb_obj_copy_ivar(VALUE dest, VALUE obj)
RUBY_ASSERT(initial_shape->type == SHAPE_T_OBJECT);

shape_to_set_on_dest = rb_shape_rebuild_shape(initial_shape, src_shape);
if (UNLIKELY(rb_shape_id(shape_to_set_on_dest) == OBJ_TOO_COMPLEX_SHAPE_ID)) {
st_table * table = rb_st_init_numtable_with_size(src_num_ivs);

rb_ivar_foreach(obj, rb_obj_evacuate_ivs_to_hash_table, (st_data_t)table);
rb_shape_set_too_complex(dest);
ROBJECT(dest)->as.heap.ivptr = (VALUE *)table;

return;
}
}

RUBY_ASSERT(src_num_ivs <= shape_to_set_on_dest->capacity);
RUBY_ASSERT(src_num_ivs <= shape_to_set_on_dest->capacity || rb_shape_id(shape_to_set_on_dest) == OBJ_TOO_COMPLEX_SHAPE_ID);
if (initial_shape->capacity < shape_to_set_on_dest->capacity) {
rb_ensure_iv_list_size(dest, initial_shape->capacity, shape_to_set_on_dest->capacity);
dest_buf = ROBJECT_IVPTR(dest);
Expand Down
10 changes: 5 additions & 5 deletions ractor.c
Expand Up @@ -2758,8 +2758,8 @@ obj_traverse_i(VALUE obj, struct obj_traverse_data *data)
if (UNLIKELY(FL_TEST_RAW(obj, FL_EXIVAR))) {
struct gen_ivtbl *ivtbl;
rb_ivar_generic_ivtbl_lookup(obj, &ivtbl);
for (uint32_t i = 0; i < ivtbl->numiv; i++) {
VALUE val = ivtbl->ivptr[i];
for (uint32_t i = 0; i < ivtbl->as.shape.numiv; i++) {
VALUE val = ivtbl->as.shape.ivptr[i];
if (!UNDEF_P(val) && obj_traverse_i(val, data)) return 1;
}
}
Expand Down Expand Up @@ -3229,9 +3229,9 @@ obj_traverse_replace_i(VALUE obj, struct obj_traverse_replace_data *data)
if (UNLIKELY(FL_TEST_RAW(obj, FL_EXIVAR))) {
struct gen_ivtbl *ivtbl;
rb_ivar_generic_ivtbl_lookup(obj, &ivtbl);
for (uint32_t i = 0; i < ivtbl->numiv; i++) {
if (!UNDEF_P(ivtbl->ivptr[i])) {
CHECK_AND_REPLACE(ivtbl->ivptr[i]);
for (uint32_t i = 0; i < ivtbl->as.shape.numiv; i++) {
if (!UNDEF_P(ivtbl->as.shape.ivptr[i])) {
CHECK_AND_REPLACE(ivtbl->as.shape.ivptr[i]);
}
}
}
Expand Down
19 changes: 14 additions & 5 deletions shape.c
Expand Up @@ -539,7 +539,7 @@ move_iv(VALUE obj, ID id, attr_index_t from, attr_index_t to)
default: {
struct gen_ivtbl *ivtbl;
rb_gen_ivtbl_get(obj, id, &ivtbl);
ivtbl->ivptr[to] = ivtbl->ivptr[from];
ivtbl->as.shape.ivptr[to] = ivtbl->as.shape.ivptr[from];
break;
}
}
Expand Down Expand Up @@ -570,7 +570,7 @@ remove_shape_recursive(VALUE obj, ID id, rb_shape_t * shape, VALUE * removed)
default: {
struct gen_ivtbl *ivtbl;
rb_gen_ivtbl_get(obj, id, &ivtbl);
*removed = ivtbl->ivptr[index];
*removed = ivtbl->as.shape.ivptr[index];
break;
}
}
Expand Down Expand Up @@ -695,8 +695,9 @@ rb_shape_transition_shape_capa_create(rb_shape_t* shape, size_t new_capacity)
ID edge_name = rb_make_temporary_id(new_capacity);
bool dont_care;
rb_shape_t * new_shape = get_next_shape_internal(shape, edge_name, SHAPE_CAPACITY_CHANGE, &dont_care, true);
RUBY_ASSERT(rb_shape_id(new_shape) != OBJ_TOO_COMPLEX_SHAPE_ID);
new_shape->capacity = (uint32_t)new_capacity;
if (rb_shape_id(new_shape) != OBJ_TOO_COMPLEX_SHAPE_ID) {
new_shape->capacity = (uint32_t)new_capacity;
}
return new_shape;
}

Expand Down Expand Up @@ -819,12 +820,18 @@ rb_shape_traverse_from_new_root(rb_shape_t *initial_shape, rb_shape_t *dest_shap
rb_shape_t *
rb_shape_rebuild_shape(rb_shape_t * initial_shape, rb_shape_t * dest_shape)
{
RUBY_ASSERT(rb_shape_id(initial_shape) != OBJ_TOO_COMPLEX_SHAPE_ID);
RUBY_ASSERT(rb_shape_id(dest_shape) != OBJ_TOO_COMPLEX_SHAPE_ID);

rb_shape_t * midway_shape;

RUBY_ASSERT(initial_shape->type == SHAPE_T_OBJECT);

if (dest_shape->type != initial_shape->type) {
midway_shape = rb_shape_rebuild_shape(initial_shape, rb_shape_get_parent(dest_shape));
if (UNLIKELY(rb_shape_id(midway_shape) == OBJ_TOO_COMPLEX_SHAPE_ID)) {
return midway_shape;
}
}
else {
midway_shape = initial_shape;
Expand All @@ -837,7 +844,9 @@ rb_shape_rebuild_shape(rb_shape_t * initial_shape, rb_shape_t * dest_shape)
midway_shape = rb_shape_transition_shape_capa(midway_shape);
}

midway_shape = rb_shape_get_next_iv_shape(midway_shape, dest_shape->edge_name);
if (LIKELY(rb_shape_id(midway_shape) != OBJ_TOO_COMPLEX_SHAPE_ID)) {
midway_shape = rb_shape_get_next_iv_shape(midway_shape, dest_shape->edge_name);
}
break;
case SHAPE_ROOT:
case SHAPE_FROZEN:
Expand Down
72 changes: 72 additions & 0 deletions test/ruby/test_shapes.rb
Expand Up @@ -184,6 +184,32 @@ def test_removing_when_too_many_ivs_on_module
assert_empty obj.instance_variables
end

def test_too_complex_geniv
assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
begin;
class TooComplex < Hash
attr_reader :very_unique
end

obj = Object.new
i = 0
while RubyVM::Shape.shapes_available > 0
obj.instance_variable_set(:"@a#{i}", 1)
i += 1
end

(RubyVM::Shape::SHAPE_MAX_VARIATIONS * 2).times do
TooComplex.new.instance_variable_set(:"@unique_#{_1}", 1)
end

tc = TooComplex.new
tc.instance_variable_set(:@very_unique, 3)
tc.instance_variable_set(:@very_unique2, 4)
assert_equal 3, tc.instance_variable_get(:@very_unique)
assert_equal 4, tc.instance_variable_get(:@very_unique2)
end;
end

def test_use_all_shapes_then_freeze
assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
begin;
Expand Down Expand Up @@ -226,6 +252,52 @@ def initialize
end;
end

def test_run_out_of_shape_generic_ivar_set
assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
begin;
class TooComplex < Hash
end

# Try to run out of shapes
o = Object.new
i = 0
while RubyVM::Shape.shapes_available > 0
o.instance_variable_set(:"@i#{i}", 1)
i += 1
end

tc = TooComplex.new
tc.instance_variable_set(:@a, 1)
tc.instance_variable_set(:@b, 2)
end;
end
Comment on lines +255 to +273
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found another crash on our CI, so I pushed a second commit to handle it.

I'm running yet another build to see if there's other cases we don't handle.


def test_run_out_of_shape_rb_obj_copy_ivar
assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
begin;
class A
def initialize
init # Avoid right sizing
end

def init
@a = @b = @c = @d = @e = @f = 1
end
end

a = A.new

o = Object.new
i = 0
while RubyVM::Shape.shapes_available > 1
o.instance_variable_set(:"@i#{i}", 1)
i += 1
end

a.dup
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So here we create a first instance of A that wasn't right sized and had to spill over in a malloc.

Then after running out of shapes we dup it, Ruby tries to rebuild the correct shape tree and run out of shapes in the middle of the process.

end;
end

def test_use_all_shapes_module
assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}")
begin;
Expand Down