<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3382/">https://reviewboard.asterisk.org/r/3382/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">To disable AO2_DEBUG / AST_DEVMODE could we just modify '#if defined(AO2_DEBUG)' to say '#if 0'. This would make it easier to review the weak reference changes, and make the review closer to the changes that you actually want to make. It might even be better if you just link to a separate patch for the performance testing changes, since these changes will not be commited.
For moving things to astobj2_private.h, astobj2.c is a large file so I like this idea in principle. This is something I'd like to see in a separate review, unless others speak up on this review against the move. Very little effort will be needed to review the technical correctness of the move on it's own, but combining with the changes for weak references makes both difficult to review.
I think the current implementation has a race between grabbing a weak reference and destroying the object. For example:
Thread 1: We unref obj1, new refcount is 0.
Thread 1: Call obj1->priv_data.destructor_fn.
Thread 2: Grab obj1 from a weak ref container. This succeeds, refcount is now 1.
Thread 1: Returns from obj1->priv_date.destructor_fn, clears AO2 magic value, clears weak refs, frees obj1;
Thread 2: Uses the already free'd obj1.
Thread 2: If using the free'd obj1 doesn't cause a crash, releasing the ref will.
This is a difficult issue to solve. The possibility that an object can be in multiple weak lists compounds this problem, since you can't atomically remove an object from multiple lists. I think this might be solvable if you don't enable weakness on the container side, but instead use a weak reference proxy object. This idea of a proxy object is inspired by https://developer.mozilla.org/en-US/docs/Weak_reference. Keep in mind Firefox has garbage collection, so I can't really point to that as a proven working example, since the delay caused by garbage collection could be partly responsible for making that safe. It's also possible that it's not actually safe in Firefox.</pre>
<br />
<p>- Corey Farrell</p>
<br />
<p>On March 23rd, 2014, 2 p.m. EDT, George Joseph wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Joshua Colp and rmudgett.</div>
<div>By George Joseph.</div>
<p style="color: grey;"><i>Updated March 23, 2014, 2 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>branches/12/tests/test_astobj2_thrash.c <span style="color: grey">(411013)</span></li>
<li>branches/12/tests/test_astobj2.c <span style="color: grey">(411013)</span></li>
<li>branches/12/main/astobj2_private.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>branches/12/main/astobj2.c <span style="color: grey">(411013)</span></li>
<li>branches/12/include/asterisk/astobj2.h <span style="color: grey">(411013)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3382/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>