[Asterisk-code-review] res pjsip registrar: Improve performance on inbound handling. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/9802 )

Change subject: res_pjsip_registrar: Improve performance on inbound handling.
......................................................................


Patch Set 1: Code-Review-1

(11 comments)

https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c
File res/res_pjsip_registrar.c:

https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@131
PS1, Line 131: static int registrar_validate_contacts(const pjsip_rx_data *rdata, pj_pool_t *pool, struct ao2_container *contacts,
             : 	struct ast_sip_aor *aor, int *added, int *updated, int *deleted)
You need to either pass in the permanent contacts count or use the aor to determine the static contacts count line in register_aor_core().


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@151
PS1, Line 151: 			*deleted = ao2_container_count(contacts);
You need to remove the permanent contacts count from the returned deleted count.


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@178
PS1, Line 178: 		existing = ao2_callback(contacts, 0, registrar_find_contact, &details);
             : 		ao2_cleanup(existing);
Permanent contacts should not affect the added, updated, deleted counts returned.


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@263
PS1, Line 263: 	if (!path_str) {
Pre-existing bug.  We are checking the wrong thing.

if (!*path_str) {
}


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@538
PS1, Line 538: 		"Contact Comparison", 256, 256);
Should change the initial size to 1024 so we have a larger initial block to carve memory out of without allocating from the heap.  Reseting the pool frees any increment and specially allocated large blocks that were allocated.  Also the initial block has the pool overhead data automatically carved out of it.


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@563
PS1, Line 563: 	/* If there are any permanent contacts configured on the AOR we need to take them into
             : 	 * account when counting contacts.
             : 	 */
             : 	if (aor->permanent_contacts) {
             : 		permanent = ao2_container_count(aor->permanent_contacts);
             : 	}
This is only used when we are not keeping track of existing contacts.


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@578
PS1, Line 578: 			ast_sorcery_object_id_sort, ast_sorcery_object_id_compare);
Why are you sorting the container?


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@658
PS1, Line 658: 		contact = ao2_callback(contacts, OBJ_UNLINK, registrar_find_contact, &details);
We'll unlink permanent contacts here too!  We'll have to put it back into the container later.


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@724
PS1, Line 724: 		} else if (expiration) {
Since we may have unlinked a permanent contact we should add it back to the contacts container then go on to the expiration else-if.

if (!contact) {
} else if (is_contact_static(contact) {
  /* Put it back into the container */
  ao2_link(contacts, contact);
} else if (expiration) {
...


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@794
PS1, Line 794: 		/* We reset the pool so we don't need to allocate additional blocks of memory on the next run */
             : 		pj_pool_reset(details.pool);
There are several continue statements in this loop after the pool is used by the ao2_callback(registrar_find_contact) line.  How about changing to a for loop and reset the pool in the increment statement?  That way your comment is true in more cases.


https://gerrit.asterisk.org/#/c/9802/1/res/res_pjsip_registrar.c@807
PS1, Line 807: 	 * The contacts container holds the current contacst of the AOR.
s/contacst/contacts/



-- 
To view, visit https://gerrit.asterisk.org/9802
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I1a78b2d46f9a2045dbbff1a3fd6dba84b612b3cb
Gerrit-Change-Number: 9802
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 02 Aug 2018 20:17:09 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180802/e0637de2/attachment.html>