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

Convert free_at_safepoint from a vector to a linked list and eliminate its associated mutex #1649

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nwc10
Copy link
Contributor

@nwc10 nwc10 commented Jan 19, 2022

  • move our atomic operations macros to a new header src/atomics.h
  • convert free_at_safepoint from a vector to a linked list
  • eliminate the mutex mutex_free_at_safepoint by using MVM_casptr
  • If we have C11 atomics use atomic_exchange_explicit instead of MVM_casptr

The commit that converts free_at_safepoint to a linked list is code @MasterDuke17 was working on as part of investigating eliminating the FSA

My subsequent changes assume that the pointer generated by the expression STABLE(type) can't change (outside of the GC) - is it correct that st is never reassigned?

nwc10 and others added 4 commits January 18, 2022 22:18
They were split in two places in moar.h - now they are in one place.
Include the file at the earlier point in moar.h - this effectively moves
the definition of various macros ahead of most headers, which is needed to
implement the next commit.
Re-order `MVM_6model_set_debug_name` to move code before or after the
critical section where `mutex_free_at_safepoint` is locked. In particular,
this moves `MVM_string_utf8_encode_C_string` outside of the critical
section, which is arguably a fix for a subtle bug, as that function has
code paths that could throw an exception.
`MVM_6model_set_debug_name` changes the pointer stored at
`STABLE(type)->debug_name`.

Instead of doing the pointer in a critical section while holding a mutex,
we use an atomic Compare And Swap to exchange the pointer, and explicitly
handle the two possibilities (swap successful, swap failed).
@niner
Copy link
Contributor

niner commented Jan 19, 2022 via email

@nwc10
Copy link
Contributor Author

nwc10 commented Jan 19, 2022

My subsequent changes assume that the pointer generated by the expression STABLE(type) can't change (outside of the GC) - is it correct that st is never reassigned? You can view, comment on, or merge this pull
Unfortunately it isn't. Rebless changes the STABLE: https://github.com/MoarVM/MoarVM/blob/master/src/6model/reprs/P6opaque.c#L1358

Hmm, interesting.

I think (stress think) that actually that means that the existing code in MVM_6model_set_debug_name is buggy, because it does this:

/* Set the debug name on a type. */
void MVM_6model_set_debug_name(MVMThreadContext *tc, MVMObject *type, MVMString *name) {
    char *orig_debug_name;
    uv_mutex_lock(&(tc->instance->mutex_free_at_safepoint));
    orig_debug_name = STABLE(type)->debug_name;
    if (orig_debug_name)
        MVM_free_at_safepoint(tc, orig_debug_name);
    STABLE(type)->debug_name = name && MVM_string_graphs(tc, name)
        ? MVM_string_utf8_encode_C_string(tc, name)
        : NULL;
    uv_mutex_unlock(&(tc->instance->mutex_free_at_safepoint));
}

and sure, it is holding the mutex mutex_free_at_safepoint while it does this, but that code in P6opaque is not, meaning that if both are executing in different threads, then the LVALUE expression STABLE(type)->debug_name might change.

Whereas my final version with this:

    void *old_debug_name
        = (void *) atomic_exchange_explicit(&STABLE(type)->debug_name, new_debug_name, memory_order_relaxed);

I think can't. Sure, it can't know which STABLE is is altering (the one before P6opaque's rebless or the one after it) but whichever it changes will remain self consistent (and no memory will leak or get double freed)

@jnthn - have we uncovered a race condition here?

@niner
Copy link
Contributor

niner commented Jan 19, 2022 via email

If we have C11 atomics we can use a single `atomic_exchange_explicit` in
`MVM_6model_set_debug_name` to update STABLE(type)->debug_name.
`libatomic_ops` doesn't have any equivalent to this, hence there we still
have to use 2 atomic operations to implement the swap.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants