Skip to content

Commit bb5ef94

Browse files
luke-gruberluke-gruber
committed
Ractors: fixes for parallel const_set/remove_const and const_get
1 parent f062dbb commit bb5ef94

File tree

3 files changed

+83
-59
lines changed

3 files changed

+83
-59
lines changed

‎constant.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ void rb_free_const_table(struct rb_id_table *tbl);
4444
VALUE rb_const_source_location(VALUE, ID);
4545

4646
int rb_autoloading_value(VALUE mod, ID id, VALUE *value, rb_const_flag_t *flag);
47-
rb_const_entry_t *rb_const_lookup(VALUE klass, ID id);
47+
rb_const_entry_t *rb_const_lookup(VALUE klass, ID id, rb_const_entry_t *entry_out);
4848
VALUE rb_public_const_get_at(VALUE klass, ID id);
4949
VALUE rb_public_const_get_from(VALUE klass, ID id);
5050
int rb_public_const_defined_from(VALUE klass, ID id);

‎variable.c‎

Lines changed: 78 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -2949,9 +2949,10 @@ static VALUE
29492949
autoload_synchronized(VALUE _arguments)
29502950
{
29512951
struct autoload_arguments *arguments = (struct autoload_arguments *)_arguments;
2952+
rb_const_entry_t constant_entry = {0};
29522953

2953-
rb_const_entry_t *constant_entry = rb_const_lookup(arguments->module, arguments->name);
2954-
if (constant_entry && !UNDEF_P(constant_entry->value)) {
2954+
rb_const_entry_t *ce = rb_const_lookup(arguments->module, arguments->name, &constant_entry);
2955+
if (ce && !UNDEF_P(constant_entry.value)) {
29552956
return Qfalse;
29562957
}
29572958

@@ -3152,13 +3153,15 @@ autoloading_const_entry(VALUE mod, ID id)
31523153
return 0;
31533154
}
31543155

3156+
31553157
static int
31563158
autoload_defined_p(VALUE mod, ID id)
31573159
{
3158-
rb_const_entry_t *ce = rb_const_lookup(mod, id);
3160+
rb_const_entry_t ce_out = {0};
3161+
rb_const_entry_t *ce = rb_const_lookup(mod, id, &ce_out);
31593162

31603163
// If there is no constant or the constant is not undefined (special marker for autoloading):
3161-
if (!ce || !UNDEF_P(ce->value)) {
3164+
if (!ce || !UNDEF_P(ce_out.value)) {
31623165
// We are not autoloading:
31633166
return 0;
31643167
}
@@ -3347,11 +3350,12 @@ autoload_try_load(VALUE _arguments)
33473350
struct autoload_load_arguments *arguments = (struct autoload_load_arguments*)_arguments;
33483351

33493352
VALUE result = autoload_feature_require(_arguments);
3353+
rb_const_entry_t ce_out = {0};
33503354

33513355
// After we loaded the feature, if the constant is not defined, we remove it completely:
3352-
rb_const_entry_t *ce = rb_const_lookup(arguments->module, arguments->name);
3356+
rb_const_entry_t *ce = rb_const_lookup(arguments->module, arguments->name, &ce_out);
33533357

3354-
if (!ce || UNDEF_P(ce->value)) {
3358+
if (!ce || UNDEF_P(ce_out.value)) {
33553359
result = Qfalse;
33563360

33573361
rb_const_remove(arguments->module, arguments->name);
@@ -3383,10 +3387,11 @@ autoload_try_load(VALUE _arguments)
33833387
VALUE
33843388
rb_autoload_load(VALUE module, ID name)
33853389
{
3386-
rb_const_entry_t *ce = rb_const_lookup(module, name);
3390+
rb_const_entry_t ce_out = {0};
3391+
rb_const_entry_t *ce = rb_const_lookup(module, name, &ce_out);
33873392

33883393
// We bail out as early as possible without any synchronisation:
3389-
if (!ce || !UNDEF_P(ce->value)) {
3394+
if (!ce || !UNDEF_P(ce_out.value)) {
33903395
return Qfalse;
33913396
}
33923397

@@ -3404,7 +3409,7 @@ rb_autoload_load(VALUE module, ID name)
34043409
// This confirms whether autoloading is required or not:
34053410
if (autoload_const_value == Qfalse) return autoload_const_value;
34063411

3407-
arguments.flag = ce->flag & (CONST_DEPRECATED | CONST_VISIBILITY_MASK);
3412+
arguments.flag = ce_out.flag & (CONST_DEPRECATED | CONST_VISIBILITY_MASK);
34083413

34093414
// Only one thread will enter here at a time:
34103415
VALUE result = rb_mutex_synchronize(arguments.mutex, autoload_try_load, (VALUE)&arguments);
@@ -3475,6 +3480,7 @@ rb_const_search_from(VALUE klass, ID id, int exclude, int recurse, int visibilit
34753480
{
34763481
VALUE value, current;
34773482
bool first_iteration = true;
3483+
rb_const_entry_t ce_out = {0};
34783484

34793485
for (current = klass;
34803486
RTEST(current);
@@ -3496,13 +3502,13 @@ rb_const_search_from(VALUE klass, ID id, int exclude, int recurse, int visibilit
34963502
if (BUILTIN_TYPE(tmp) == T_ICLASS) tmp = RBASIC(tmp)->klass;
34973503

34983504
// Do the lookup. Loop in case of autoload.
3499-
while ((ce = rb_const_lookup(tmp, id))) {
3500-
if (visibility && RB_CONST_PRIVATE_P(ce)) {
3505+
while ((ce = rb_const_lookup(tmp, id, &ce_out))) {
3506+
if (visibility && RB_CONST_PRIVATE_P(&ce_out)) {
35013507
GET_EC()->private_const_reference = tmp;
35023508
return Qundef;
35033509
}
3504-
rb_const_warn_if_deprecated(ce, tmp, id);
3505-
value = ce->value;
3510+
rb_const_warn_if_deprecated(&ce_out, tmp, id);
3511+
value = ce_out.value;
35063512
if (UNDEF_P(value)) {
35073513
struct autoload_const *ac;
35083514
if (am == tmp) break;
@@ -3582,16 +3588,17 @@ rb_const_location_from(VALUE klass, ID id, int exclude, int recurse, int visibil
35823588
{
35833589
while (RTEST(klass)) {
35843590
rb_const_entry_t *ce;
3591+
rb_const_entry_t ce_out = {0};
35853592

3586-
while ((ce = rb_const_lookup(klass, id))) {
3587-
if (visibility && RB_CONST_PRIVATE_P(ce)) {
3593+
while ((ce = rb_const_lookup(klass, id, &ce_out))) {
3594+
if (visibility && RB_CONST_PRIVATE_P(&ce_out)) {
35883595
return Qnil;
35893596
}
35903597
if (exclude && klass == rb_cObject) {
35913598
goto not_found;
35923599
}
35933600

3594-
if (UNDEF_P(ce->value)) { // autoload
3601+
if (UNDEF_P(ce_out.value)) { // autoload
35953602
VALUE autoload_const_value = autoload_data(klass, id);
35963603
if (RTEST(autoload_const_value)) {
35973604
struct autoload_const *autoload_const;
@@ -3603,8 +3610,8 @@ rb_const_location_from(VALUE klass, ID id, int exclude, int recurse, int visibil
36033610
}
36043611
}
36053612

3606-
if (NIL_P(ce->file)) return rb_ary_new();
3607-
return rb_assoc_new(ce->file, INT2NUM(ce->line));
3613+
if (NIL_P(ce_out.file)) return rb_ary_new();
3614+
return rb_assoc_new(ce_out.file, INT2NUM(ce_out.line));
36083615
}
36093616
if (!recurse) break;
36103617
klass = RCLASS_SUPER(klass);
@@ -3661,18 +3668,19 @@ rb_mod_remove_const(VALUE mod, VALUE name)
36613668
return rb_const_remove(mod, id);
36623669
}
36633670

3664-
static rb_const_entry_t * const_lookup(struct rb_id_table *tbl, ID id);
3671+
static rb_const_entry_t * const_lookup(struct rb_id_table *tbl, ID id, rb_const_entry_t *ce_out);
36653672

36663673
VALUE
36673674
rb_const_remove(VALUE mod, ID id)
36683675
{
36693676
VALUE val;
36703677
rb_const_entry_t *ce;
3678+
rb_const_entry_t ce_out = {0};
36713679

36723680
rb_check_frozen(mod);
36733681

36743682
RB_VM_LOCKING() {
3675-
ce = rb_const_lookup(mod, id);
3683+
ce = rb_const_lookup(mod, id, &ce_out);
36763684
if (!ce || !rb_id_table_delete(RCLASS_WRITABLE_CONST_TBL(mod), id)) {
36773685
if (rb_const_defined_at(mod, id)) {
36783686
rb_name_err_raise("cannot remove %2$s::%1$s", mod, ID2SYM(id));
@@ -3684,14 +3692,14 @@ rb_const_remove(VALUE mod, ID id)
36843692
rb_const_warn_if_deprecated(ce, mod, id);
36853693
rb_clear_constant_cache_for_id(id);
36863694

3687-
val = ce->value;
3695+
val = ce_out.value;
36883696

36893697
if (UNDEF_P(val)) {
36903698
autoload_delete(mod, id);
36913699
val = Qnil;
36923700
}
36933701

3694-
if (ce != const_lookup(RCLASS_PRIME_CONST_TBL(mod), id)) {
3702+
if (ce != const_lookup(RCLASS_PRIME_CONST_TBL(mod), id, NULL)) {
36953703
ruby_xfree(ce);
36963704
}
36973705
// else - skip free'ing the ce because it still exists in the prime classext
@@ -3835,15 +3843,16 @@ rb_const_defined_0(VALUE klass, ID id, int exclude, int recurse, int visibility)
38353843
VALUE tmp;
38363844
int mod_retry = 0;
38373845
rb_const_entry_t *ce;
3846+
rb_const_entry_t ce_out = {0};
38383847

38393848
tmp = klass;
38403849
retry:
38413850
while (tmp) {
3842-
if ((ce = rb_const_lookup(tmp, id))) {
3843-
if (visibility && RB_CONST_PRIVATE_P(ce)) {
3851+
if ((ce = rb_const_lookup(tmp, id, &ce_out))) {
3852+
if (visibility && RB_CONST_PRIVATE_P(&ce_out)) {
38443853
return (int)Qfalse;
38453854
}
3846-
if (UNDEF_P(ce->value) && !check_autoload_required(tmp, id, 0) &&
3855+
if (UNDEF_P(ce_out.value) && !check_autoload_required(tmp, id, 0) &&
38473856
!rb_autoloading_value(tmp, id, NULL, NULL))
38483857
return (int)Qfalse;
38493858

@@ -3971,8 +3980,8 @@ const_set(VALUE klass, ID id, VALUE val)
39713980
RCLASS_WRITE_CONST_TBL(klass, tbl, false);
39723981
rb_clear_constant_cache_for_id(id);
39733982
ce = ZALLOC(rb_const_entry_t);
3974-
rb_id_table_insert(tbl, id, (VALUE)ce);
39753983
setup_const_entry(ce, klass, val, CONST_PUBLIC);
3984+
rb_id_table_insert(tbl, id, (VALUE)ce);
39763985
}
39773986
else {
39783987
struct autoload_const ac = {
@@ -3985,6 +3994,7 @@ const_set(VALUE klass, ID id, VALUE val)
39853994
}
39863995
}
39873996

3997+
// TODO: I think this needs to be locked
39883998
/*
39893999
* Resolve and cache class name immediately to resolve ambiguity
39904000
* and avoid order-dependency on const_tbl
@@ -4049,6 +4059,8 @@ const_tbl_update(struct autoload_const *ac, int autoload_force)
40494059
rb_const_flag_t visibility = ac->flag;
40504060
rb_const_entry_t *ce;
40514061

4062+
ASSERT_vm_locking();
4063+
40524064
if (rb_id_table_lookup(tbl, id, &value)) {
40534065
ce = (rb_const_entry_t *)value;
40544066
if (UNDEF_P(ce->value)) {
@@ -4095,8 +4107,8 @@ const_tbl_update(struct autoload_const *ac, int autoload_force)
40954107
rb_clear_constant_cache_for_id(id);
40964108

40974109
ce = ZALLOC(rb_const_entry_t);
4098-
rb_id_table_insert(tbl, id, (VALUE)ce);
40994110
setup_const_entry(ce, klass, val, visibility);
4111+
rb_id_table_insert(tbl, id, (VALUE)ce);
41004112
}
41014113
}
41024114

@@ -4144,29 +4156,31 @@ set_const_visibility(VALUE mod, int argc, const VALUE *argv,
41444156
return;
41454157
}
41464158

4147-
for (i = 0; i < argc; i++) {
4148-
struct autoload_const *ac;
4149-
VALUE val = argv[i];
4150-
id = rb_check_id(&val);
4151-
if (!id) {
4152-
undefined_constant(mod, val);
4153-
}
4154-
if ((ce = rb_const_lookup(mod, id))) {
4155-
ce->flag &= ~mask;
4156-
ce->flag |= flag;
4157-
if (UNDEF_P(ce->value)) {
4158-
struct autoload_data *ele;
4159-
4160-
ele = autoload_data_for_named_constant(mod, id, &ac);
4161-
if (ele) {
4162-
ac->flag &= ~mask;
4163-
ac->flag |= flag;
4159+
RB_VM_LOCKING() {
4160+
for (i = 0; i < argc; i++) {
4161+
struct autoload_const *ac;
4162+
VALUE val = argv[i];
4163+
id = rb_check_id(&val);
4164+
if (!id) {
4165+
undefined_constant(mod, val);
4166+
}
4167+
if ((ce = rb_const_lookup(mod, id, NULL))) {
4168+
ce->flag &= ~mask;
4169+
ce->flag |= flag;
4170+
if (UNDEF_P(ce->value)) {
4171+
struct autoload_data *ele;
4172+
4173+
ele = autoload_data_for_named_constant(mod, id, &ac);
4174+
if (ele) {
4175+
ac->flag &= ~mask;
4176+
ac->flag |= flag;
4177+
}
41644178
}
4179+
rb_clear_constant_cache_for_id(id);
4180+
}
4181+
else {
4182+
undefined_constant(mod, ID2SYM(id));
41654183
}
4166-
rb_clear_constant_cache_for_id(id);
4167-
}
4168-
else {
4169-
undefined_constant(mod, ID2SYM(id));
41704184
}
41714185
}
41724186
}
@@ -4182,10 +4196,12 @@ rb_deprecate_constant(VALUE mod, const char *name)
41824196
if (!(id = rb_check_id_cstr(name, len, NULL))) {
41834197
undefined_constant(mod, rb_fstring_new(name, len));
41844198
}
4185-
if (!(ce = rb_const_lookup(mod, id))) {
4186-
undefined_constant(mod, ID2SYM(id));
4199+
RB_VM_LOCKING() {
4200+
if (!(ce = rb_const_lookup(mod, id, NULL))) {
4201+
undefined_constant(mod, ID2SYM(id));
4202+
}
4203+
ce->flag |= CONST_DEPRECATED;
41874204
}
4188-
ce->flag |= CONST_DEPRECATED;
41894205
}
41904206

41914207
/*
@@ -4742,23 +4758,30 @@ rb_fields_tbl_copy(VALUE dst, VALUE src)
47424758
}
47434759

47444760
static rb_const_entry_t *
4745-
const_lookup(struct rb_id_table *tbl, ID id)
4761+
const_lookup(struct rb_id_table *tbl, ID id, rb_const_entry_t *entry_out)
47464762
{
47474763
if (tbl) {
4764+
rb_const_entry_t *ce;
47484765
VALUE val;
47494766
bool r;
47504767
RB_VM_LOCKING() {
47514768
r = rb_id_table_lookup(tbl, id, &val);
4769+
if (r) {
4770+
ce = (rb_const_entry_t*)val;
4771+
if (entry_out) *entry_out = *ce;
4772+
}
47524773
}
47534774

4754-
if (r) return (rb_const_entry_t *)val;
4775+
if (r) {
4776+
return ce;
4777+
}
47554778
}
47564779
return NULL;
47574780
}
47584781

47594782
rb_const_entry_t *
4760-
rb_const_lookup(VALUE klass, ID id)
4783+
rb_const_lookup(VALUE klass, ID id, rb_const_entry_t *entry_out)
47614784
{
4762-
return const_lookup(RCLASS_CONST_TBL(klass), id);
4785+
return const_lookup(RCLASS_CONST_TBL(klass), id, entry_out);
47634786
}
47644787

‎vm_insnhelper.c‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1092,10 +1092,11 @@ vm_get_ev_const(rb_execution_context_t *ec, VALUE orig_klass, ID id, bool allow_
10921092
if (!NIL_P(klass)) {
10931093
VALUE av, am = 0;
10941094
rb_const_entry_t *ce;
1095+
rb_const_entry_t ce_out = {0};
10951096
search_continue:
1096-
if ((ce = rb_const_lookup(klass, id))) {
1097-
rb_const_warn_if_deprecated(ce, klass, id);
1098-
val = ce->value;
1097+
if ((ce = rb_const_lookup(klass, id, &ce_out))) {
1098+
rb_const_warn_if_deprecated(&ce_out, klass, id);
1099+
val = ce_out.value;
10991100
if (UNDEF_P(val)) {
11001101
if (am == klass) break;
11011102
am = klass;

0 commit comments

Comments
 (0)