Skip to content

Commit

Permalink
Merge pull request #495 from MoarVM/fix-inline-and-threads
Browse files Browse the repository at this point in the history 
Fix bugs in interaction between inlining, GC, and threads
  • Loading branch information
@jnthn
jnthn committed Jan 13, 2017
2 parents d9482b9 + ef4d6a7 commit d1da1ba
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 49 deletions.
38 changes: 24 additions & 14 deletions src/6model/reprs/MVMCompUnit.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,12 @@ static MVMObject * type_object_for(MVMThreadContext *tc, MVMObject *HOW) {

/* Initializes a new instance. */
static void initialize(MVMThreadContext *tc, MVMSTable *st, MVMObject *root, void *data) {
MVMROOT(tc, root, {
MVMCompUnit *cu = (MVMCompUnit *)root;
MVMROOT(tc, cu, {
MVMObject *rm = MVM_repr_alloc_init(tc, tc->instance->boot_types.BOOTReentrantMutex);
MVM_ASSIGN_REF(tc, &(root->header), ((MVMCompUnit *)root)->body.update_mutex, rm);
MVM_ASSIGN_REF(tc, &(root->header), cu->body.deserialize_frame_mutex, rm);
cu->body.inline_tweak_mutex = MVM_malloc(sizeof(uv_mutex_t));
uv_mutex_init(cu->body.inline_tweak_mutex);
});
}

Expand Down Expand Up @@ -62,7 +65,7 @@ static void gc_mark(MVMThreadContext *tc, MVMSTable *st, void *data, MVMGCWorkli
/* Unresolved sc bodies' handles are marked by the GC instance root marking. */
}

MVM_gc_worklist_add(tc, worklist, &body->update_mutex);
MVM_gc_worklist_add(tc, worklist, &body->deserialize_frame_mutex);

/* Add various other referenced strings, etc. */
MVM_gc_worklist_add(tc, worklist, &body->hll_name);
Expand All @@ -72,22 +75,29 @@ static void gc_mark(MVMThreadContext *tc, MVMSTable *st, void *data, MVMGCWorkli
/* Called by the VM in order to free memory associated with this object. */
static void gc_free(MVMThreadContext *tc, MVMObject *obj) {
MVMCompUnitBody *body = &((MVMCompUnit *)obj)->body;
int i;

int i;
for (i = 0; i < body->num_callsites; i++) {
MVMCallsite *cs = body->callsites[i];

if (cs->is_interned) {
continue;
}

MVM_callsite_destroy(cs);
if (!cs->is_interned)
MVM_callsite_destroy(cs);
}

uv_mutex_destroy(body->inline_tweak_mutex);
MVM_free(body->inline_tweak_mutex);
MVM_free(body->coderefs);
MVM_free(body->callsites);
MVM_free(body->extops);
MVM_free(body->strings);
if (body->callsites)
MVM_fixed_size_free(tc, tc->instance->fsa,
body->num_callsites * sizeof(MVMCallsite *),
body->callsites);
if (body->extops)
MVM_fixed_size_free(tc, tc->instance->fsa,
body->num_extops * sizeof(MVMExtOpRecord),
body->extops);
if (body->strings)
MVM_fixed_size_free(tc, tc->instance->fsa,
body->num_strings * sizeof(MVMString *),
body->strings);
MVM_free(body->scs);
MVM_free(body->scs_to_resolve);
MVM_free(body->sc_handle_idxs);
Expand Down Expand Up @@ -194,7 +204,7 @@ static void describe_refs(MVMThreadContext *tc, MVMHeapSnapshotState *ss, MVMSTa
(MVMCollectable *)body->scs[i], "Serialization context dependency");

MVM_profile_heap_add_collectable_rel_const_cstr(tc, ss,
(MVMCollectable *)body->update_mutex, "Update_mutex");
(MVMCollectable *)body->deserialize_frame_mutex, "Update_mutex");

/* Add various other referenced strings, etc. */
MVM_profile_heap_add_collectable_rel_const_cstr(tc, ss,
Expand Down
15 changes: 10 additions & 5 deletions src/6model/reprs/MVMCompUnit.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,16 @@ struct MVMCompUnitBody {
/* Handle, if any, associated with a mapped file. */
void *handle;

/* MVMReentrantLock to be taken if we want to add extra string,
* callsite, or coderef constants to the pools (done during
* inlining) or when we finish deserializing a frame, thus
* vivifying its lexicals. */
MVMObject *update_mutex;
/* Unmanaged (so not GC-aware) mutex taken if we want to add extra string,
* callsite, extop, or coderef constants to the pools. This is done in
* some cases of cross-compilation-unit inlining. We are never at risk of
* recursion on this mutex, and since spesh can never GC it's important we
* do not use a GC-aware mutex, which could trigger GC. */
uv_mutex_t *inline_tweak_mutex;

/* MVMReentrantLock to be taken when we want to finish deserializing a
* frame inside of the compilation unit. */
MVMObject *deserialize_frame_mutex;

/* Version of the bytecode format we deserialized this comp unit from. */
MVMuint16 bytecode_version;
Expand Down
19 changes: 11 additions & 8 deletions src/core/bytecode.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,8 @@ static MVMExtOpRecord * deserialize_extop_records(MVMThreadContext *tc, MVMCompU
if (num == 0)
return NULL;

extops = MVM_calloc(num, sizeof *extops);
extops = MVM_fixed_size_alloc_zeroed(tc, tc->instance->fsa,
num * sizeof(MVMExtOpRecord));

pos = rs->extop_seg;
for (i = 0; i < num; i++) {
Expand Down Expand Up @@ -581,11 +582,11 @@ void MVM_bytecode_finish_frame(MVMThreadContext *tc, MVMCompUnit *cu,
return;

/* Acquire the update mutex on the CompUnit. */
MVM_reentrantmutex_lock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
MVM_reentrantmutex_lock(tc, (MVMReentrantMutex *)cu->body.deserialize_frame_mutex);

/* Ensure no other thread has done this for us in the mean time. */
if (sf->body.fully_deserialized) {
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.deserialize_frame_mutex);
return;
}

Expand Down Expand Up @@ -666,7 +667,7 @@ void MVM_bytecode_finish_frame(MVMThreadContext *tc, MVMCompUnit *cu,
MVMuint16 flags = read_int16(pos, 2);

if (lex_idx >= sf->body.num_lexicals) {
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.deserialize_frame_mutex);
MVM_exception_throw_adhoc(tc, "Lexical index out of bounds: %d > %d", lex_idx, sf->body.num_lexicals);
}

Expand All @@ -676,7 +677,7 @@ void MVM_bytecode_finish_frame(MVMThreadContext *tc, MVMCompUnit *cu,
* can wait. */
MVMSerializationContext *sc = MVM_sc_get_sc(tc, cu, read_int32(pos, 4));
if (sc == NULL) {
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.deserialize_frame_mutex);
MVM_exception_throw_adhoc(tc, "SC not yet resolved; lookup failed");
}
MVM_ASSIGN_REF(tc, &(sf->common.header), sf->body.static_env[lex_idx].o,
Expand All @@ -689,7 +690,7 @@ void MVM_bytecode_finish_frame(MVMThreadContext *tc, MVMCompUnit *cu,
sf->body.fully_deserialized = 1;

/* Release the update mutex again */
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.deserialize_frame_mutex);
}

/* Gets the SC reference for a given static lexical var for
Expand Down Expand Up @@ -724,7 +725,8 @@ static MVMCallsite ** deserialize_callsites(MVMThreadContext *tc, MVMCompUnit *c
/* Allocate space for callsites. */
if (rs->expected_callsites == 0)
return NULL;
callsites = MVM_malloc(sizeof(MVMCallsite *) * rs->expected_callsites);
callsites = MVM_fixed_size_alloc(tc, tc->instance->fsa,
sizeof(MVMCallsite *) * rs->expected_callsites);

/* Load callsites. */
pos = rs->callsite_seg;
Expand Down Expand Up @@ -851,7 +853,8 @@ void MVM_bytecode_unpack(MVMThreadContext *tc, MVMCompUnit *cu) {
rs = dissect_bytecode(tc, cu);

/* Allocate space for the strings heap; we deserialize it lazily. */
cu_body->strings = MVM_calloc(rs->expected_strings, sizeof(MVMString *));
cu_body->strings = MVM_fixed_size_alloc_zeroed(tc, tc->instance->fsa,
rs->expected_strings * sizeof(MVMString *));
cu_body->num_strings = rs->expected_strings;
cu_body->orig_strings = rs->expected_strings;
cu_body->string_heap_fast_table = MVM_calloc(
Expand Down
32 changes: 22 additions & 10 deletions src/core/compunit.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ MVMuint16 MVM_cu_callsite_add(MVMThreadContext *tc, MVMCompUnit *cu, MVMCallsite
MVMuint16 found = 0;
MVMuint16 idx;

MVM_reentrantmutex_lock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
uv_mutex_lock(cu->body.inline_tweak_mutex);

/* See if we already know this callsite. */
for (idx = 0; idx < cu->body.num_callsites; idx++)
Expand All @@ -111,14 +111,20 @@ MVMuint16 MVM_cu_callsite_add(MVMThreadContext *tc, MVMCompUnit *cu, MVMCallsite
}
if (!found) {
/* Not known; let's add it. */
size_t orig_size = cu->body.num_callsites * sizeof(MVMCallsite *);
size_t new_size = (cu->body.num_callsites + 1) * sizeof(MVMCallsite *);
MVMCallsite **new_callsites = MVM_fixed_size_alloc(tc, tc->instance->fsa, new_size);
memcpy(new_callsites, cu->body.callsites, orig_size);
idx = cu->body.num_callsites;
cu->body.callsites = MVM_realloc(cu->body.callsites,
(idx + 1) * sizeof(MVMCallsite *));
cu->body.callsites[idx] = MVM_callsite_copy(tc, cs);
new_callsites[idx] = MVM_callsite_copy(tc, cs);
if (cu->body.callsites)
MVM_fixed_size_free_at_safepoint(tc, tc->instance->fsa, orig_size,
cu->body.callsites);
cu->body.callsites = new_callsites;
cu->body.num_callsites++;
}

MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
uv_mutex_unlock(cu->body.inline_tweak_mutex);

return idx;
}
Expand All @@ -128,7 +134,7 @@ MVMuint32 MVM_cu_string_add(MVMThreadContext *tc, MVMCompUnit *cu, MVMString *st
MVMuint32 found = 0;
MVMuint32 idx;

MVM_reentrantmutex_lock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
uv_mutex_lock(cu->body.inline_tweak_mutex);

/* See if we already know this string; only consider those added already by
* inline, since we don't intern and don't want this to be costly to hunt. */
Expand All @@ -139,14 +145,20 @@ MVMuint32 MVM_cu_string_add(MVMThreadContext *tc, MVMCompUnit *cu, MVMString *st
}
if (!found) {
/* Not known; let's add it. */
size_t orig_size = cu->body.num_strings * sizeof(MVMString *);
size_t new_size = (cu->body.num_strings + 1) * sizeof(MVMString *);
MVMString **new_strings = MVM_fixed_size_alloc(tc, tc->instance->fsa, new_size);
memcpy(new_strings, cu->body.strings, orig_size);
idx = cu->body.num_strings;
cu->body.strings = MVM_realloc(cu->body.strings,
(idx + 1) * sizeof(MVMString *));
cu->body.strings[idx] = str;
new_strings[idx] = str;
if (cu->body.strings)
MVM_fixed_size_free_at_safepoint(tc, tc->instance->fsa, orig_size,
cu->body.strings);
cu->body.strings = new_strings;
cu->body.num_strings++;
}

MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
uv_mutex_unlock(cu->body.inline_tweak_mutex);

return idx;
}
Expand Down
4 changes: 2 additions & 2 deletions src/core/frame.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ static void prepare_and_verify_static_frame(MVMThreadContext *tc, MVMStaticFrame

/* Take compilation unit lock, to make sure we don't race to do the
* frame preparation/verification work. */
MVM_reentrantmutex_lock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
MVM_reentrantmutex_lock(tc, (MVMReentrantMutex *)cu->body.deserialize_frame_mutex);
if (static_frame->body.instrumentation_level == 0) {
/* Work size is number of locals/registers plus size of the maximum
* call site argument list. */
Expand Down Expand Up @@ -64,7 +64,7 @@ static void prepare_and_verify_static_frame(MVMThreadContext *tc, MVMStaticFrame
}

/* Unlock, now we're finished. */
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.update_mutex);
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)cu->body.deserialize_frame_mutex);
}

/* When we don't match the current instrumentation level, we hit this. It may
Expand Down
24 changes: 14 additions & 10 deletions src/spesh/inline.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ static void demand_extop(MVMThreadContext *tc, MVMCompUnit *target_cu, MVMCompUn
MVMExtOpRecord *extops;
MVMuint16 i, num_extops;

MVM_reentrantmutex_lock(tc, (MVMReentrantMutex *)target_cu->body.update_mutex);
uv_mutex_lock(target_cu->body.inline_tweak_mutex);

/* See if the target compunit already has the extop. */
extops = target_cu->body.extops;
num_extops = target_cu->body.num_extops;
for (i = 0; i < num_extops; i++)
if (extops[i].info == info) {
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)target_cu->body.update_mutex);
uv_mutex_unlock(target_cu->body.inline_tweak_mutex);
return;
}

Expand All @@ -22,20 +22,24 @@ static void demand_extop(MVMThreadContext *tc, MVMCompUnit *target_cu, MVMCompUn
num_extops = source_cu->body.num_extops;
for (i = 0; i < num_extops; i++) {
if (extops[i].info == info) {
MVMuint32 size = (target_cu->body.num_extops + 1) * sizeof(MVMExtOpRecord);
target_cu->body.extops = target_cu->body.extops
? MVM_realloc(target_cu->body.extops, size)
: MVM_malloc(size);
memcpy(&target_cu->body.extops[target_cu->body.num_extops],
&extops[i], sizeof(MVMExtOpRecord));
MVMuint32 orig_size = target_cu->body.num_extops * sizeof(MVMExtOpRecord);
MVMuint32 new_size = (target_cu->body.num_extops + 1) * sizeof(MVMExtOpRecord);
MVMExtOpRecord *new_extops = MVM_fixed_size_alloc(tc,
tc->instance->fsa, new_size);
memcpy(new_extops, target_cu->body.extops, orig_size);
memcpy(&new_extops[target_cu->body.num_extops], &extops[i], sizeof(MVMExtOpRecord));
if (target_cu->body.extops)
MVM_fixed_size_free_at_safepoint(tc, tc->instance->fsa, orig_size,
target_cu->body.extops);
target_cu->body.extops = new_extops;
target_cu->body.num_extops++;
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)target_cu->body.update_mutex);
uv_mutex_unlock(target_cu->body.inline_tweak_mutex);
return;
}
}

/* Didn't find it; should be impossible. */
MVM_reentrantmutex_unlock(tc, (MVMReentrantMutex *)target_cu->body.update_mutex);
uv_mutex_unlock(target_cu->body.inline_tweak_mutex);
MVM_oops(tc, "Spesh: inline failed to find source CU extop entry");
}

Expand Down

0 comments on commit d1da1ba

Please sign in to comment.