[asterisk-dev] [Code Review] 3576: astobj2: Split hash and rbtree impls into their own source files.
rmudgett
reviewboard at asterisk.org
Wed Jun 4 16:47:10 CDT 2014
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3576/#review12048
-----------------------------------------------------------
branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment22009>
Drat! Splitting astobj2 makes the utils directory no longer compile. That's going to suck a bit getting those hacked up utilities to compile again.
branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment21999>
This needs to be moved above handle_astobj2_stats().
branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment22001>
Remove. This is now setup as an atexit by container_init().
branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment22005>
Move this back to astobj2.c. This is an ao2 primitive test function only used by handle_astobj2_test() in astobj2.c.
branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment22002>
This only needs to be conditionaled by AST_DEVMODE now.
branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment22003>
This only needs to be conditionaled by AST_DEVMODE now.
branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment21997>
This is part of the ao2 primitive code and needs to stay with astobj2.c primitive cleanup code.
branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment22000>
This is part of the ao2 primitive code and needs to stay with astobj2.c primitive init code.
branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment21998>
This is part of the ao2 primitive code and needs to stay with astobj2.c primitive init code.
branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment22004>
This only needs to be conditionaled by AST_DEVMODE now.
branches/12/main/astobj2_container_private.h
<https://reviewboard.asterisk.org/r/3576/#comment22008>
Most of the contents of astobj2_private.h should not be here.
These should be put back into astobj2.c where they were:
struct __priv_data
AO2_MAGIC
struct astobj2
struct ao2_lock_priv
struct astobj2_lock
struct ao2_rwlock_priv
struct astobj2_rwlock
INTERNAL_OBJ_MUTEX()
INTERNAL_OBJ_RWLOCK()
EXTERNAL_OBJ()
INTERNAL_OBJ() needs to be kept inside astobj2.c as well. It also should not be declared as an inline function. Let the compiler decide if it wants to inline it. There are only two use cases of it outside of astobj2.c for very specific purposes.
1) It is used as a sanity check that the pointer passed into the container routines is a valid ao2 object. Create an is_ao2_object() internal API call for these uses.
2) It is used to fetch the ao2 options flags from a container object for cloning the container. Create a new PUBLIC API call in astobj2.c "unsigned int ao2_options_get(void *obj)" for these uses.
branches/12/main/astobj2_container_private.h
<https://reviewboard.asterisk.org/r/3576/#comment21993>
Does not need to be included. Only the hash container uses.
branches/12/main/astobj2_container_private.h
<https://reviewboard.asterisk.org/r/3576/#comment21995>
I'd group these prototypes with container_cleanup() and container_init() below.
branches/12/main/astobj2_container_private.h
<https://reviewboard.asterisk.org/r/3576/#comment22007>
These can also be grouped with the other prototypes at the bottom.
branches/12/main/astobj2_container_private.h
<https://reviewboard.asterisk.org/r/3576/#comment22006>
This is an ao2 primitive test function only used by handle_astobj2_test() in astobj2.c.
branches/12/main/astobj2_private.h
<https://reviewboard.asterisk.org/r/3576/#comment21992>
Does not need to be included.
branches/12/main/astobj2_private.h
<https://reviewboard.asterisk.org/r/3576/#comment21994>
There must be only one of these variables for the counts to work.
extern struct ao2_stats ao2;
And in astobj2.c:
struct ao2_stats ao2;
branches/12/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment21989>
tobj[] needs to be initialized to zero before any possible goto test_cleanup.
tobj2 does not need to be initialized.
branches/12/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment21990>
use ao2_cleanup()
branches/12/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment21991>
use ao2_cleanup()
branches/12/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment21988>
Compiler error: ast_tvdiff_ms(ast_tvnow(), start) does not match the format string. ast_tvdiff_ms() returns int64_t. Probably best to cast it to (unsigned long).
- rmudgett
On May 31, 2014, 12:54 a.m., George Joseph wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3576/
> -----------------------------------------------------------
>
> (Updated May 31, 2014, 12:54 a.m.)
>
>
> Review request for Asterisk Developers and rmudgett.
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> In preparation for weak-reference containers, and because it makes the existing code easier to read and maintain, I've split the astobj2 common structure and enum definitions and prototypes into astobj2_private.h, the hash table implementation into astobj2_hash.c, and the rbtree implementation into astobj2_rbtree.c. All of the public functions remain in astobj2.c.
>
> A few functions (adjust_lock, container_destruct, container_destruct_debug) needed to have their static modifiers removed so they'd be visible from the other object files but other than that there were NO functional changes, no logic changes, etc.
>
> EDIT:..
> Also added a basic test to the test framework to monitor performance impacts as changes are made to astobj2.
>
>
> Diffs
> -----
>
> branches/12/tests/test_astobj2.c 414969
> branches/12/main/astobj2_rbtree.c PRE-CREATION
> branches/12/main/astobj2_private.h PRE-CREATION
> branches/12/main/astobj2_hash.c PRE-CREATION
> branches/12/main/astobj2_container_private.h PRE-CREATION
> branches/12/main/astobj2_container.c PRE-CREATION
> branches/12/main/astobj2.c 414969
>
> Diff: https://reviewboard.asterisk.org/r/3576/diff/
>
>
> Testing
> -------
>
> I used both the test framework and the test suite. For the test suite, I used channels/pjsip since that exercises sorcery significantly and that in turn exercises astobj2.
>
> All tests that worked before the change worked after the change.
>
> Before...
>
> Test Framework
> 393 Test(s) Executed 393 Passed 0 Failed
>
> Test Suite
> tests/channels/pjsip/
> Tests: 88 Passed: 87 Failed: 1
> FAILED: tests/channels/pjsip/dialplan_functions/pjsip_endpoint
>
> After...
>
> Test Framework
> 393 Test(s) Executed 393 Passed 0 Failed
>
> Test Suite
> tests/channels/pjsip/
> Tests: 88 Passed: 87 Failed: 1
> FAILED: tests/channels/pjsip/dialplan_functions/pjsip_endpoint
>
> Not sure why the pjsip_endpoint function is failing but it's not this patch's fault.
>
>
> Thanks,
>
> George Joseph
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140604/c97d3eba/attachment-0001.html>
More information about the asterisk-dev
mailing list