623993 - AsymmGC - Guarded RC write barrier (infrastructure)

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P3)

Description

[Simon Wilkinson]

Attachment: Initial Patch. MQ rev 192

Bug 623802 introduces a thread identity test into RCObject::IncrementRef() and RCObject::DecrementRef() thus:

REALLY_INLINE void IncrementRef()
{
    GC* const gc = GC::GetGC(this);
#ifdef MMGC_THREADSAFE_GC
    if (!gc->onThread()) {
        SetShared();
        return;
    }
#endif

</snip>

This patch provides the infrastructure to remove this test at build time when it is known that an RC write barrier is only reachable by a GC-owner thread (which, hopefully, is most of them, when considering background compilation and async XML parsing).

The general idea is to make RCObject::IncrementRef() and RCObject::DecrementRef() function templates, e.g.:

template <bool THREAD_GUARD>
REALLY_INLINE void IncrementRef()
{
#ifdef MMGC_THREADSAFE_GC_FAST_WBRC_WITH_CONTEXT
    if (THREAD_GUARD && !GC::GetGC(this)->onThread()) {
        SetShared();
        return;
    } else {
        GCAssertMsg(GC::GetGC(this)->onThread(), "Attempt to inc ref unguarded on non-GC-owner thread\n");
    }
#endif

Following equivalent templatization of the necessary plumbing functions, all smart-pointer RC writer barrier classes are then extended with an equivalent template parameter, e.g.:

template<class T, bool checkMissingWB=true, bool THREAD_GUARD>
class RCPtr

template<class T, bool THREAD_GUARD>
class WriteBarrierRC

template<bool THREAD_GUARD>
class AtomWB

... and all manual RC write barrier functions are also modified to be function templates with an equivalent template parameter, e.g:

template<bool THREAD_GUARD> void GC::privateWriteBarrierRC(const void *container, const void *address, const void *value);

We can then define two versions of RC barrier macros, 'GUARDED' and unguarded, e.g.:

#define DRCWB(type) MMgc::WriteBarrierRC< type, false >
#define DRCWB_GUARDED(type) MMgc::WriteBarrierRC< type, true >

#define WBRC(gc, container, addr, value) gc->privateWriteBarrierRC<false>(container, addr, (const void *) (value))
#define WBRC_GUARDED(gc, container, addr, value) gc->privateWriteBarrierRC<true>(container, addr, (const void *) (value))

With these new macros, only those fields and manual barrier locations that are reachable by VM-threads require the GUARDED versions. All other barriers can be left as they are.
Note, this means that *all* player classes can use unchanged, unguarded macro declarations. Also all player code can use unchanged, unguarded manual barriers, and all JITed code can use unguarded barrier versions.
Hence, all that we must worry about are those barriers in the VM implementation and the VM's builtins.

In later patches for async XML and the background compiler, the necessary barriers are modified to GUARDED versions. In this patch, everything is unguarded.

Note that this feature is currently ifdef'ed by MMGC_THREADSAFE_GC_FAST_WBRC_WITH_CONTEXT, not MMGC_THREADSAFE_GC.
Also note that most of the templatization is done via the macros TEMPLATE_THREAD_GUARD, APPLY_THREAD_GUARD and PARAM_THREAD_GUARD defined in MMgc.h. This is purely to reduce the #ifdef-ing.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

This part of the changeset:

+        TEMPLATE_THREAD_GUARD
         REALLY_INLINE void IncrementRef()
         {
-            GC* const gc = GC::GetGC(this);
-#ifdef MMGC_THREADSAFE_GC
-            if (!gc->onThread()) {
+#ifdef MMGC_THREADSAFE_GC_FAST_WBRC_WITH_CONTEXT
+            if (THREAD_GUARD && !GC::GetGC(this)->onThread()) {
                 SetShared();
                 return;
+            } else {
+                GCAssertMsg(GC::GetGC(this)->onThread(), "Attempt to inc ref unguarded on non-GC-owner thread\n");
             }
 #endif

seems flawed, in that if I remove the MMGC_THREADSAFE_GC_FAST_WBRC_WITH_CONTEXT feature, but leave in the MMGC_THREADSAFE_GC feature, the behavior of IncrementRef does not return to its prior form (of unconditionally checking the current thread and setting the shared bit if appropriate).

(This burned me pretty bad yesterday because I've been trying to isolate where performance changes come from, and thought feature-toggling would suffice to revert patches like this.)

There's a similar problem in DecrementRef.

Comment 2

[Simon Wilkinson]

(In reply to comment #1)
if I remove the MMGC_THREADSAFE_GC_FAST_WBRC_WITH_CONTEXT
> feature, but leave in the MMGC_THREADSAFE_GC feature, the behavior of
> IncrementRef does not return to its prior form (of unconditionally checking the
> current thread and setting the shared bit if appropriate).
> 
> (This burned me pretty bad yesterday because I've been trying to isolate where
> performance changes come from, and thought feature-toggling would suffice to
> revert patches like this.)
> 
> There's a similar problem in DecrementRef.

If you refer to bug 621086, which adds the feature defines, then you'll see that eventually MMGC_THREADSAFE_GC is the only define that will be used throughout. At the moment, there's still MMGC_THREADSAFE_GC_FAST_WBRC_WITH_CONTEXT, MMGC_THREADSAFE_GC_FAST_ALLOC and a few other legacy defines used through the patches. These are in the process of being removed.

I know previously you've been used to feature-toggling via the defines, but now this is no longer the case. Pushing and popping patches is the way. All the MMGC_THREADSAFE_GC* features must always be defined (in the interim until there is only MMGC_THREADSAFE_GC)

My bad for not making this clear before.

Comment 3

[Simon Wilkinson]

Attachment: MQ rev 226. Added profiling support to MMGC_REFCOUNT_PROFILING

Comment 4

[Simon Wilkinson]

Attachment: Rebased. TR rev 6090. Patch queue rev 261

Starting the discussion on this patch, and bug 623802, as both are going to
have to undergo a non-trival rebase on TR rev 6127, i.e. they have to be
converted to GCMember rather than DWBRC.
I'd rather do that conversion with input from the rest of the team.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

I think I've made this comment elsewhere, but since its not currently on this ticket, I'll repeat myself: Rather than a bool for the THREAD_GUARD template parameter, I'd prefer to see an enum with meaningful names, like { kMayBeCalledFromBackgroundThread, kDefinitelyCalledFromOwnerThread }, or something along those lines.

It may not matter much for this patch though, since so much of it is putting in the plumbing to thread through the current value for the parameter without care for which value it has.

Should it perhaps be factored into two patches:
* the first that just inserts plumbing but has no conceptual additions because it just uses kMayBeCalledFromBackgroundThread at all of the root entry points, and
* the second that changes the key root entry points so that they now say kDefinitelyCalledFromOwnerThread ?

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

(F- because I think like I should be included in a future feedback session; am not saying the idea is bad.  But really, the meanings of an "F-" feels very squishy to me ...)

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 521959 [details] [diff] [review]
Rebased. TR rev 6090. Patch queue rev 261

(changing F- to F+; I think "F-" has a different meaning than "R-" and I don't want my entry at the top to influence other reviewers when they look at this.)

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #5)
> I think I've made this comment elsewhere, but since its not currently on this
> ticket, I'll repeat myself: Rather than a bool for the THREAD_GUARD template
> parameter, I'd prefer to see an enum with meaningful names, like {
> kMayBeCalledFromBackgroundThread, kDefinitelyCalledFromOwnerThread }, or
> something along those lines.

As follow-up to this: you did abstract out the sites where the boolean flag was used and give them the names like privateWriteBarrierRC_NoThreadGuard / privateWriteBarrierRC_ThreadGuard.  That's better than a boolean, but is it the right mental model for the caller?

I think anyone who sees those names is going to have to take up time wondering about when they are supposed to use ThreadGuard versus NoThreadGuard.  I think names more like the ones I put up above have more client-oriented semantics and less internal-implementation-oriented semantics, but that's just my opinion.  Am curious to see what other mmgc folks say.

Comment 9

[Simon Wilkinson]

(In reply to comment #8)
> (In reply to comment #5)
> > I think I've made this comment elsewhere, but since its not currently on this
> > ticket, I'll repeat myself: Rather than a bool for the THREAD_GUARD template
> > parameter, I'd prefer to see an enum with meaningful names, like {
> > kMayBeCalledFromBackgroundThread, kDefinitelyCalledFromOwnerThread }, or
> > something along those lines.
> 
> As follow-up to this: you did abstract out the sites where the boolean flag was
> used and give them the names like privateWriteBarrierRC_NoThreadGuard /
> privateWriteBarrierRC_ThreadGuard.  That's better than a boolean, but is it the
> right mental model for the caller?
> 
> I think anyone who sees those names is going to have to take up time wondering
> about when they are supposed to use ThreadGuard versus NoThreadGuard.  I think
> names more like the ones I put up above have more client-oriented semantics and
> less internal-implementation-oriented semantics, but that's just my opinion. 
> Am curious to see what other mmgc folks say.

The functions with a _NoThreadGuard or _ThreadGuard suffix were intended as
out-of-line helpers for existing out-of-line (FASTCALLed) barriers that are
templatized and inlined by this patch.

I've renamed them in the latest rev to reflect this more clearly.

Comment 10

[Simon Wilkinson]

Attachment: Updated to enum template param. TR rev 6090. Patch queue rev 270

Comment 11

[Simon Wilkinson]

Attachment: Updated(2) to enum template param. TR rev 6090. Patch queue rev 271

Comment 12

[Simon Wilkinson]

Attachment: Updated(3) to enum template param. TR rev 6090. Patch queue rev 272

Comment 13

[Simon Wilkinson]

(In reply to comment #5)
> I think I've made this comment elsewhere, but since its not currently on this
> ticket, I'll repeat myself: Rather than a bool for the THREAD_GUARD template
> parameter, I'd prefer to see an enum with meaningful names, like {
> kMayBeCalledFromBackgroundThread, kDefinitelyCalledFromOwnerThread }, or
> something along those lines.
> 
> It may not matter much for this patch though, since so much of it is putting in
> the plumbing to thread through the current value for the parameter without care
> for which value it has.
> 
> Should it perhaps be factored into two patches:
> * the first that just inserts plumbing but has no conceptual additions because
> it just uses kMayBeCalledFromBackgroundThread at all of the root entry points,
> and
> * the second that changes the key root entry points so that they now say
> kDefinitelyCalledFromOwnerThread ?

In the latest rev I've changed the bool template param to:
    enum ThreadGuard
    {
        GC_OWNER_ONLY,
        GC_OWNER_OR_ASYMM
    };

Comment 14

[Simon Wilkinson]

Comment on attachment 525574 [details] [diff] [review]
Updated(3) to enum template param. TR rev 6090. Patch queue rev 272


Asking for feedback again, in view of the necessary rebase on TR rev 6127 (i.e. converting to GCMember rather than DWBRC)

Comment 15

[Dan Smith]

Retargeting to Dolores.