[asterisk-dev] [Code Review] 3382: RFC: Weak Reference Containers

Mon Mar 24 16:21:04 CDT 2014

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3382/#review11355
-----------------------------------------------------------

* I think you need to add an assert to prevent ao2 objects that don't have a lock from participating in weak ref containers.  Otherwise, the object's weak ref list has no reentrancy protection.

* Container node destructors assume that the container is already write locked because the destructor unlinks the node from the container's structure.

* Red-black trees are not handling the weak ref nodes that have a destroying object very well.  The container could still use the key to determine which child to go down for searches since the object still exists in the container.  The container just cannot return the object.  Inserting duplicate objects could consider the destroying object still in the tree.

* The AO2_CONTAINER_ALLOC_OPT_DUPS_REPLACE case where the found node is for a destroying object needs examining for red-black trees.  In this case I think that the insert needs to consider the node as not a match and continue looking.

* The AO2_CONTAINER_ALLOC_OPT_DUPS_REPLACE needs to update the old linked object's weak-ref list when another object replaces it.  Weak-ref containers probably need to treat replace as a delete followed by an insert.  It may be best to not allow it because destroying objects make the replace optimization of assuming one and only one object with that key exists no longer valid.

branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3382/#comment21001>

    Curly on the same line as AST_LIST_TRAVERSE.

branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3382/#comment21000>

    The node's container must be write locked before unreffing here in case this is the last reference of the container's node.

    The container node normally has one reference (the container itself).  The other times the container node is referenced is when an iterator is currently at that node in its iteration.
    The object's weak ref list may need to have a ref to the container to prevent a race between the container destruction and the object destruction.

branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3382/#comment21003>

    tag, file, line, and func need to be passed in to give credit in user code where an object was linked into the container.

    Probably need to add tag, file, line, and func parameters to internal_ao2_unlink_node() for the same reason.

branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3382/#comment20998>

    You need to treat the ao2 objects as if they had rwlocks and use ao2_wrlock() instead of ao2_lock().  Using ao2_wrlock() over ao2_lock() is really a formality since ao2_lock() means ao2_wrlock().

branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3382/#comment21002>

    Add a note:

    The node's container must be write locked before calling if the AO2_UNLINK_NODE_UNREF_NODE flag is passed in.

branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3382/#comment20990>

    Just use AST_LIST_REMOVE() here.

    AST_LIST_REMOVE() does its own traverse to find the node in the list to remove it.  You don't need an explicit AST_LIST_TRAVERSE().

branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3382/#comment20999>

    The case lines are indented too far.

branches/12/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/3382/#comment20996>

    Guidelines:

    These mostly appear to be copy-paste repetition:
    switch (type) {

    while () {

    case TEST_CONTAINER_HASH:

- rmudgett

On March 24, 2014, 12:17 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3382/
> -----------------------------------------------------------
> 
> (Updated March 24, 2014, 12:17 p.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This is a Request For Comments patch that adds weak reference capability to astobj2 containers.
> 
> Current (Strong-ref) behavior:  A container (list, hash or rbtree) increments an object's ref count when linked and decrements an object's ref count when unlinked.  Therefore the object can never be destroyed while it is an entry in a container unless someone accidentally decrements the object's ref count too many times.  In this case, the object is invalid but the container doesn't know it.
> 
> Weak-ref behavior:  A container (list, hash or rbtree) allocated with the option AO2_CONTAINER_ALLOC_OPT_WEAK_REF neither increments an object's ref count when linked nor decrements an object's ref count when unlinked.  If the object's ref count can therefore validly reach 0 in which case the object is automatically and cleanly removed from any weak-ref container it may be in.
> 
> Use case:  The possible need for weak-ref containers came up during the development of the Sorcery registry.  The first call to sorcery_open from a module should create a new sorcery instance and subsequent calls from that same module should use that instance.  The last call to close should free that instance.  With a strong-ref container however, the container itself always has a a reference to the instance so it doesn't get destroyed without special code to check the ref count on each call to close.  
> 
> Implementation:  astobj2.priv_data now has a linked list that contains the weak-ref container nodes that reference the object.  When an object is added to a weak-ref container, the container node is added to the list instead of the node incrementing the object's ref count.  Similarly, when an object is removed from a weak-ref container the node is removed from the linked list instead of the object's ref count being decremented.  If an object's ref count reaches 0 the object's linked list is traversed and the corresponding nodes are cleared effectively removing the object from the container.  NOTE:  An object's ref count is still incremented as the result of a retrieval (find, iterate, etc) from a weak-ref container.
> 
> Backwards compatibility:  Unless the AO2_CONTAINER_ALLOC_OPT_WEAK_REF flag was set on container allocation, all container operations remain exactly as they are today and nothing prevents an object from being a member of both strong and weak ref containers at the same time.
> 
> Performance implications:  Due to code reorganization, the performance of strong-ref containers is actually microscopically BETTER than the unmodified code and the performance of weak-ref containers is even better than that.  In other words, the performance of the default behavior was not penalized by the addition of the new feature.  
> 
> Code reorganization.  I moved all of the structure definitions in astobj2.c to astobj2_private.h.  This makes astobj2.c much easier to read and debug.  I was also able to push down some implementation specific code to the hash and rbtree functions and pull up some duplicated code from the hash and rbtree functions.  
> 
> Patch notes:  The patch file itself might be a little hard to read because of the reorganization so applying the patch is your best bet for detailed review.  The patch will apply cleanly to both branches/12 and trunk. Also, the patch disables AO2_DEBUG and AST_DEVMODE in astobj2 so that performance can be tested while still keeping the test framework.  The final patch will remove the disables.
> 
> 
> Diffs
> -----
> 
>   branches/12/tests/test_astobj2_thrash.c 411013 
>   branches/12/tests/test_astobj2.c 411013 
>   branches/12/main/astobj2.c 411013 
>   branches/12/include/asterisk/astobj2.h 411013 
> 
> Diff: https://reviewboard.asterisk.org/r/3382/diff/
> 
> 
> Testing
> -------
> 
> All of the existing unit tests in test_astobj2 pass including the thrash test.
> 
> I added 7 additional unit tests specifically for the weak-ref implementation including a performance comparison test that compares both strong and weak ref implementations.  A thrash test was also added for weak-ref.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140324/6f7affb9/attachment-0001.html>