[asterisk-dev] [Code Review] 3593: astobj2: Additional refactoring to push impl specific code down into the impls.

[rmudgett]

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


Most of the changes to eliminate the RTTI code can be eliminated or simplified by adding two vtable callbacks that are present only if AO2_DEBUG is enabled instead of the one required unlink_node_fn callback.

* Add unlink_node_stat_fn and link_node_stat_fn callbacks that correspond to hash_ao2_link_node_stat() and hash_ao2_unlink_node_stat() respectively.
* Pull the hash_ao2_unlink_node() and rb_ao2_unlink_node() code up into the common container code as __container_unlink_node() which will call the unlink_node_stat_fn callback if present.
* The internal_ao2_link() function call will call the link_node_stat_fn callback if present instead of the original RTTI switch code.

Suggested prototype:
int __container_unlink_node(struct ao2_container_node *node, uint32_t flags, const char *tag, const char *file, int line, const char *func)

The flags parameter is as you have already defined it (enum ao2_unlink_node_flags).
The file, line, and func parameters are used when the external object is unreffed so the user's code will be indicated in the ref logs.
We probably should also have a macro version that supplies the file, line, and func parameters for use by the node destructor calls to simplify things.

Several of the findings below are obsoleted with the suggestion above.


branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3593/#comment22283>

    The container should be checked if the node was inserted and if the object replaced an existing object.  The specific container may have replaced the wrong object and blown the container integrity.



branches/12/main/astobj2_container_private.h
<https://reviewboard.asterisk.org/r/3593/#comment22269>

    (internal) is redundant with the \internal doxygen tag.



branches/12/main/astobj2_hash.c
<https://reviewboard.asterisk.org/r/3593/#comment22270>

    (internal) is redundant with the \internal doxygen tag.



branches/12/main/astobj2_rbtree.c
<https://reviewboard.asterisk.org/r/3593/#comment22273>

    (internal) is redundant with the \internal doxygen tag.



branches/12/main/astobj2_rbtree.c
<https://reviewboard.asterisk.org/r/3593/#comment22278>

    This is common code to any astobj2 container and should be pulled back up to the general container code in the AO2_CONTAINER_INSERT_NODE_INSERTED case.
    
    The goto inserted changes in this function could be reverted as a result.
    
    The hash container specific hash_ao2_link_node_stat() call that is done in the corresponding location of hash_ao2_insert_node() would still be done there.



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

    You should keep the container_options parameter.  How the container handles duplicate objects does affect performance.
    
    The AO2_CONTAINER_ALLOC_OPT_DUPS_OBJ_REJECT should generally provide better performance than the other duplicate options.


- rmudgett


On June 10, 2014, 9:42 a.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3593/
> -----------------------------------------------------------
> 
> (Updated June 10, 2014, 9:42 a.m.)
> 
> 
> Review request for Asterisk Developers and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Move some implementation specific code from astobj2_container.c into astobj2_hash.c and astobj2_rbtree.c.  This completely removes the need for astobj2_container to switch on RTTI and it no longer has any knowledge of the implementation details.  The RTTI itself is no longer needed but I don't want to remove it until weak reference containers are added in case I need it back again.  If not, I'll remove it then.
> 
> 
> Diffs
> -----
> 
>   branches/12/tests/test_astobj2.c 415657 
>   branches/12/main/astobj2_rbtree.c 415657 
>   branches/12/main/astobj2_private.h 415657 
>   branches/12/main/astobj2_hash.c 415657 
>   branches/12/main/astobj2_container_private.h 415657 
>   branches/12/main/astobj2_container.c 415657 
>   branches/12/build_tools/cflags.xml 415657 
> 
> Diff: https://reviewboard.asterisk.org/r/3593/diff/
> 
> 
> Testing
> -------
> 
> Ran all the test framework tests plus the "astobj2 test" test.
> Ran testsuite tests/channels/pjsip.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

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