[Asterisk-code-review] main/pbx: Improve performance of dialplan reloads with large... (asterisk[13])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: main/pbx: Improve performance of dialplan reloads with large numbers of hints
......................................................................


Patch Set 1:

(7 comments)

https://gerrit.asterisk.org/#/c/312/1/main/pbx.c
File main/pbx.c:

Line 1051: 	AST_VECTOR(, char *) devices; /*!< Devices associated with the hint */
> Fixed.
Actually, nope. This is malloc'd; as such, it is a char *.


Line 1085: static int hintdevice_hash_cb(const void *obj, const int flags)
> ao2 template from wiki
Fixed.


Line 1110: static int hintdevice_cmp_multiple(void *obj, void *arg, int flags)
> This should be using the ao2 "Unsorted container cmp function" template fro
Fixed.


Line 1152: 		dev_iter = ao2_t_callback(hintdevices, OBJ_SEARCH_KEY | OBJ_MULTIPLE,
         : 			hintdevice_cmp_multiple, device, "find devices in container");
> He is using OBJ_MULTIPLE. But I still think CMP_STOP isn't useful here beca
See, but that actually *is* the problem.

Say I have 20000 hints, each with a single device watcher. That's 20000 entries in that hash table. With 563 buckets, I could have 50 collisions in that bucket. With OBJ_MULTIPLE, it will give me back all 50 collisions, even though I don't care about that - I *only* want the unique match of device/hint.

Hence, CMP_STOP is appropriate here:
1) There can be multiple matches on the device name in hintdevices, much less collisions.
2) I only want to remove one of them.


Line 1235: 		AST_VECTOR_APPEND(&hint->devices, device_name);
> device_name will leak if AST_VECTOR_APPEND() returns non-zero.  It means th
Fixed.


Line 1235: 		AST_VECTOR_APPEND(&hint->devices, device_name);
> This should be done before linking into the hintdevices container in case A
Fixed.


Line 12584: static void pbx_shutdown(void)
> You need to ao2_container_unregister() the hints, hintdevices, and statecbs
Fixed.


-- 
To view, visit https://gerrit.asterisk.org/312
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iedfc97a69d21070c50fca42275d7b3e714e59360
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes