Re: [PATCH] RefCount API shuffling

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.

> +/**
> + * 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."

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/

> +private:
> + mutable unsigned count_;
> +};

Missing documentation. Consider "///< lock level" or similar.

> 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.

> 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.

> 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.

> * update the HttpMsg locking API to utilize LockableObject for
> reference tracking
> * replace the HttpMsgPointerT locking API with RefCount<HttpMsg>

These should be pretty straightforward.

> * 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.

Thank you,

Alex.
Received on Fri Oct 26 2012 - 16:12:58 MDT