[asterisk-dev] [Code Review] 3513: Weak Reference Containers

[George Joseph]

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

(Updated June 20, 2014, 1:42 p.m.)


Status
------

This change has been discarded.


Review request for Asterisk Developers, Joshua Colp and rmudgett.


Repository: Asterisk


Description
-------

Add weak reference capability to hash and list astobj2 containers.

Current (Strong-reference) 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-reference behavior:  A container (list or hash) 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.  The object's ref count can therefore validly reach 0 while still in a weak-ref container in which case the object is automatically and cleanly removed from any weak-ref container it may be in.  Because of the complexity of the red-black tree implementation, it isn't eligible for weak-reference behavior.

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 reference to the instance so it doesn't get destroyed without special code to check the ref count on each call to close.  The same thing happens with the pjsip cli command registry.

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.  The test framework now has a /main/astobj2/perf/ category to show relative performance.  NOTE:  Previously, when the test framework was enabled AO2_DEBUG was also enabled but this was skewing the results so I removed that auto-enablement of AO2_DEBUG.  The test framework never needed it.

The original RFC review request was here... https://reviewboard.asterisk.org/r/3382/
The major changes from the RFC are that a spinlock was added around the object's ref-counter operations and the rbtree's are now excluded from being weak-reference containers.


Diffs
-----

  branches/12/tests/test_astobj2_thrash.c 413157 
  branches/12/tests/test_astobj2.c 413157 
  branches/12/main/astobj2.c 413157 
  branches/12/include/asterisk/astobj2.h 413157 

Diff: https://reviewboard.asterisk.org/r/3513/diff/


Testing
-------

7 new tests were added to the existing astobj2 test framework.  All tests pass.

The testsuite is a little more challenging but I at least made sure that all the test that passed before the change still passed after the change.


Thanks,

George Joseph

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140620/b541da78/attachment.html>