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
