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

[George Joseph]

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

(Updated March 24, 2014, 11:17 a.m.)


Review request for Asterisk Developers, Joshua Colp and rmudgett.


Changes
-------

Added a 'destroying' flag to astobj2 that gets set immediately after internal_ao2_ref recognizes that an object is being destroyed.  Subsequent hash and rbtree find/iterate functions will skip that node as though it were an empty node.  I think this addresses Corey's concern over the race condition.


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 (updated)
-----

  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/8726cbde/attachment.html>