Re: [PATCH] RefCount API shuffling

On 27/10/2012 5:12 a.m., Alex Rousskov wrote:
> On 10/26/2012 05:21 AM, Amos Jeffries wrote:
>> This polishes the RefCount API a bit.
>>
>> * Shuffle RefCount.h and its unit-tests into src/base/
> Great!
>
>> * Reworks struct Refcountable_ into class LockableObject in its own
>> header file
>> + changing the reference counter accessors to a lock()/unlock() names
>> + some minor symbol updates of code directly utilizing the
>> RefCountable_ members
> Please s/LockableObject/Lock/g.

Okay. Done.

>
>> +/**
>> + * Base class for all reference tracking types.
>> + * RefCount, CbData, etc
>> + *
>> + * The objects to be tracked inherit from this and
>> + * construct/destruct/invalidation of the trackign mechanism
>> + * use this interface API to maintain the locks.
>> + */
>> +class LockableObject {
> It would be nice to describe what this class is before we jump into
> describing what other classes are (or will be) using it [for]. For
> example, you can start by saying something like "Something that can be
> locked and unlocked. Supports multiple lock levels (handy for shared
> objects). Must not be destroyed in a locked state but does not require
> destruction when the lock level reaches zero."

I thought I had with that first sentence. But threshing this out a bit
more anyway as requested, and removing the specific tracking mechanisms
names.

"
  * This class presents the lock()/unlock() and counter accessors
  * interface for tracking references to some object
  *
  * The objects to be tracked inherit from this class and some
  * separate tracking mechanism is needed to takes care of how
  * the lock count affects the child objects behaviour.
  *
  * Accessors provided by this interface are not private,
  * to allow class hierarchies.
  *
  * Build with -DLOCKCOUNT_DEBUG flag to enable lock debugging
  * it is disabled by default due to the ost of debug output.
"

>
> Is the "construct/destruct/invalidation of the trackign mechanism..."
> part missing some words? I cannot parse it.
>
> "trackign" is misspelled.
>
> s/interface API/API/ or s/interface API/interface/
>
Fixed.

>
>> +private:
>> + mutable unsigned count_;
>> +};
> Missing documentation. Consider "///< lock level" or similar.
>

Done.
>
>> With this we can begin the process of replacing our multiple different
>> implementations of the reference-counting pattern using LockableObject.
> Sounds good, but I am not sure there is clear understanding and/or
> consensus of what those implementations should be, so if you are going
> to work on that, please post RFCs or sketches before spending a lot of
> cycles on large-scope changes.

Of course. That is why this patch is going in by itself. I intend to do
each tracking mechanism update as a separetely scoped patch for only
that mechanism. RefCount and CbData are relatively easy. Some like the
StoreEntry one require the existng mechanism to be extended rather than
replaced completely.

>
>> No code changes have been made. Just symbol polishing.
>> If there are no objections I will apply this in a day or two.
> No objections from me.

Okay. Once we have agreement on how to document this class (latest
proposal above) I will commit.

>
>
>> TODO: Followup stages which can build on this are (in no particular order):
>> * update the backend of CBDATA to utilize LockableObject for reference
>> tracking
> Yes, but this is one of those tricky/controversial areas I am worried
> about. I doubt cbdata API should be changed just for the sake of reusing
> Lock.

CBDATA API will not change. The change there would be "class cbdata:
public Lock" and its cbdataInternal*() updated to use the new internal
counter variable.

>
>> * update the HttpMsg locking API to utilize LockableObject for
>> reference tracking
>> * replace the HttpMsgPointerT locking API with RefCount<HttpMsg>
> These should be pretty straightforward.

It would seem so, but HttpMsgPointerT does some slightly weird semantics
like LOCK() returning the pointer to the object just locked which need
to be worked around during the change.

>
>> * replace the StoreEntry locking API and move to CbcPointer
> An even bigger mine field! Needs a lot of thought. I doubt, for example,
> that StoreEntries should be cbdata-protected.
>
>
>> I'm sure there are others floating around as well that I have not
>> encountered yet. lock/unlock seems to be a popular operation in the code.
> but often with different semantics so we must tread carefully.

Agreed.

Amos
Received on Sat Oct 27 2012 - 09:17:45 MDT