Skip to content

Commit d25b74b

Browse files
eileencodestenderlovemethodmissing
committed
Resize arrays in rb_ary_freeze and use it for freezing arrays
While working on a separate issue we found that in some cases `ary_heap_realloc` was being called on frozen arrays. To fix this, this change does the following: 1) Updates `rb_ary_freeze` to assert the type is an array, return if already frozen, and shrink the capacity if it is not embedded, shared or a shared root. 2) Replaces `rb_obj_freeze` with `rb_ary_freeze` when the object is always an array. 3) In `ary_heap_realloc`, ensure the new capa is set with `ARY_SET_CAPA`. Previously the change in capa was not set. 4) Adds an assertion to `ary_heap_realloc` that the array is not frozen. Some of this work was originally done in #2640, referencing this issue https://bugs.ruby-lang.org/issues/16291. There didn't appear to be any objections to this PR, it appears to have simply lost traction. The original PR made changes to arrays and strings at the same time, this PR only does arrays. Also it was old enough that rather than revive that branch I've made a new one. I added Lourens as co-author in addtion to Aaron who helped me with this patch. The original PR made this change for performance reasons, and while that's still true for this PR, the goal of this PR is to avoid calling `ary_heap_realloc` on frozen arrays. The capacity should be shrunk _before_ the array is frozen, not after. Co-authored-by: Aaron Patterson <tenderlove@ruby-lang.org> Co-Authored-By: methodmissing <lourens@methodmissing.com>
1 parent cee62c6 commit d25b74b

File tree

8 files changed

+29
-16
lines changed

8 files changed

+29
-16
lines changed

array.c

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,7 @@ ary_heap_free(VALUE ary)
370370
static size_t
371371
ary_heap_realloc(VALUE ary, size_t new_capa)
372372
{
373+
RUBY_ASSERT(!OBJ_FROZEN(ary));
373374
SIZED_REALLOC_N(RARRAY(ary)->as.heap.ptr, VALUE, new_capa, ARY_HEAP_CAPA(ary));
374375
ary_verify(ary);
375376

@@ -441,7 +442,10 @@ ary_shrink_capa(VALUE ary)
441442
long old_capa = ARY_HEAP_CAPA(ary);
442443
RUBY_ASSERT(!ARY_SHARED_P(ary));
443444
RUBY_ASSERT(old_capa >= capacity);
444-
if (old_capa > capacity) ary_heap_realloc(ary, capacity);
445+
if (old_capa > capacity) {
446+
size_t new_capa = ary_heap_realloc(ary, capacity);
447+
ARY_SET_CAPA(ary, new_capa);
448+
}
445449

446450
ary_verify(ary);
447451
}
@@ -639,6 +643,14 @@ ary_ensure_room_for_push(VALUE ary, long add_len)
639643
VALUE
640644
rb_ary_freeze(VALUE ary)
641645
{
646+
RUBY_ASSERT(RB_TYPE_P(ary, T_ARRAY));
647+
648+
if (OBJ_FROZEN(ary)) return ary;
649+
650+
if (!ARY_EMBED_P(ary) && !ARY_SHARED_P(ary) && !ARY_SHARED_ROOT_P(ary)) {
651+
ary_shrink_capa(ary);
652+
}
653+
642654
return rb_obj_freeze(ary);
643655
}
644656

@@ -889,7 +901,7 @@ rb_setup_fake_ary(struct RArray *fake_ary, const VALUE *list, long len, bool fre
889901
ARY_SET_PTR(ary, list);
890902
ARY_SET_HEAP_LEN(ary, len);
891903
ARY_SET_CAPA(ary, len);
892-
if (freeze) OBJ_FREEZE(ary);
904+
if (freeze) rb_ary_freeze(ary);
893905
return ary;
894906
}
895907

@@ -6458,7 +6470,7 @@ rb_ary_flatten_bang(int argc, VALUE *argv, VALUE ary)
64586470
if (result == ary) {
64596471
return Qnil;
64606472
}
6461-
if (!(mod = ARY_EMBED_P(result))) rb_obj_freeze(result);
6473+
if (!(mod = ARY_EMBED_P(result))) rb_ary_freeze(result);
64626474
rb_ary_replace(ary, result);
64636475
if (mod) ARY_SET_EMBED_LEN(result, 0);
64646476

@@ -8754,6 +8766,7 @@ Init_Array(void)
87548766
rb_define_method(rb_cArray, "one?", rb_ary_one_p, -1);
87558767
rb_define_method(rb_cArray, "dig", rb_ary_dig, -1);
87568768
rb_define_method(rb_cArray, "sum", rb_ary_sum, -1);
8769+
rb_define_method(rb_cArray, "freeze", rb_ary_freeze, 0);
87578770

87588771
rb_define_method(rb_cArray, "deconstruct", rb_ary_deconstruct, 0);
87598772
}

ast.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -769,7 +769,7 @@ ast_node_all_tokens(rb_execution_context_t *ec, VALUE self)
769769
token = rb_ary_new_from_args(4, INT2FIX(parser_token->id), ID2SYM(rb_intern(parser_token->type_name)), str, loc);
770770
rb_ary_push(all_tokens, token);
771771
}
772-
rb_obj_freeze(all_tokens);
772+
rb_ary_freeze(all_tokens);
773773

774774
return all_tokens;
775775
}

compile.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4840,7 +4840,7 @@ static_literal_value(const NODE *node, rb_iseq_t *iseq)
48404840
if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) {
48414841
VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX((int)nd_line(node)));
48424842
VALUE lit = rb_str_dup(get_string_value(node));
4843-
rb_ivar_set(lit, id_debug_created_info, rb_obj_freeze(debug_info));
4843+
rb_ivar_set(lit, id_debug_created_info, rb_ary_freeze(debug_info));
48444844
return rb_str_freeze(lit);
48454845
}
48464846
else {
@@ -10752,7 +10752,7 @@ iseq_compile_each0(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const no
1075210752
if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) {
1075310753
VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX(line));
1075410754
lit = rb_str_dup(lit);
10755-
rb_ivar_set(lit, id_debug_created_info, rb_obj_freeze(debug_info));
10755+
rb_ivar_set(lit, id_debug_created_info, rb_ary_freeze(debug_info));
1075610756
lit = rb_str_freeze(lit);
1075710757
}
1075810758
ADD_INSN1(ret, node, putobject, lit);
@@ -11321,7 +11321,7 @@ rb_insns_name_array(void)
1132111321
for (i = 0; i < VM_INSTRUCTION_SIZE; i++) {
1132211322
rb_ary_push(ary, rb_fstring_cstr(insn_name(i)));
1132311323
}
11324-
return rb_obj_freeze(ary);
11324+
return rb_ary_freeze(ary);
1132511325
}
1132611326

1132711327
static LABEL *
@@ -13709,7 +13709,7 @@ ibf_load_object_array(const struct ibf_load *load, const struct ibf_object_heade
1370913709
rb_ary_push(ary, ibf_load_object(load, index));
1371013710
}
1371113711

13712-
if (header->frozen) rb_obj_freeze(ary);
13712+
if (header->frozen) rb_ary_freeze(ary);
1371313713

1371413714
return ary;
1371513715
}

enumerator.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3181,7 +3181,7 @@ enum_chain_initialize(VALUE obj, VALUE enums)
31813181

31823182
if (!ptr) rb_raise(rb_eArgError, "unallocated chain");
31833183

3184-
ptr->enums = rb_obj_freeze(enums);
3184+
ptr->enums = rb_ary_freeze(enums);
31853185
ptr->pos = -1;
31863186

31873187
return obj;
@@ -3509,7 +3509,7 @@ enum_product_initialize(int argc, VALUE *argv, VALUE obj)
35093509

35103510
if (!ptr) rb_raise(rb_eArgError, "unallocated product");
35113511

3512-
ptr->enums = rb_obj_freeze(enums);
3512+
ptr->enums = rb_ary_freeze(enums);
35133513

35143514
return obj;
35153515
}

ext/date/date_core.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ static VALUE sym_hour, sym_min, sym_sec, sym_sec_fraction, sym_zone;
5757
#define f_add3(x,y,z) f_add(f_add(x, y), z)
5858
#define f_sub3(x,y,z) f_sub(f_sub(x, y), z)
5959

60-
#define f_frozen_ary(...) rb_obj_freeze(rb_ary_new3(__VA_ARGS__))
60+
#define f_frozen_ary(...) rb_ary_freeze(rb_ary_new3(__VA_ARGS__))
6161

6262
static VALUE date_initialize(int argc, VALUE *argv, VALUE self);
6363
static VALUE datetime_initialize(int argc, VALUE *argv, VALUE self);
@@ -9466,7 +9466,7 @@ mk_ary_of_str(long len, const char *a[])
94669466
}
94679467
rb_ary_push(o, e);
94689468
}
9469-
rb_obj_freeze(o);
9469+
rb_ary_freeze(o);
94709470
return o;
94719471
}
94729472

iseq.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -521,7 +521,7 @@ rb_iseq_pathobj_new(VALUE path, VALUE realpath)
521521
else {
522522
if (!NIL_P(realpath)) realpath = rb_fstring(realpath);
523523
pathobj = rb_ary_new_from_args(2, rb_fstring(path), realpath);
524-
rb_obj_freeze(pathobj);
524+
rb_ary_freeze(pathobj);
525525
}
526526
return pathobj;
527527
}

load.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ rb_construct_expanded_load_path(rb_vm_t *vm, enum expand_type type, int *has_rel
104104
if (NIL_P(expanded_path)) expanded_path = as_str;
105105
rb_ary_push(ary, rb_fstring(expanded_path));
106106
}
107-
rb_obj_freeze(ary);
107+
rb_ary_freeze(ary);
108108
vm->expanded_load_path = ary;
109109
rb_ary_replace(vm->load_path_snapshot, vm->load_path);
110110
}

prism_compile.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ parse_static_literal_string(rb_iseq_t *iseq, const pm_scope_node_t *scope_node,
299299
int line_number = pm_node_line_number(scope_node->parser, node);
300300
VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX(line_number));
301301
value = rb_str_dup(value);
302-
rb_ivar_set(value, id_debug_created_info, rb_obj_freeze(debug_info));
302+
rb_ivar_set(value, id_debug_created_info, rb_ary_freeze(debug_info));
303303
rb_str_freeze(value);
304304
}
305305

@@ -693,7 +693,7 @@ pm_static_literal_string(rb_iseq_t *iseq, VALUE string, int line_number)
693693
{
694694
if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) {
695695
VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX(line_number));
696-
rb_ivar_set(string, id_debug_created_info, rb_obj_freeze(debug_info));
696+
rb_ivar_set(string, id_debug_created_info, rb_ary_freeze(debug_info));
697697
return rb_str_freeze(string);
698698
}
699699
else {

0 commit comments

Comments
 (0)