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

[Mark Michelson]

Mark Michelson has posted comments on this change.

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


Patch Set 1: Code-Review-1

(6 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 */
Seems like this could be const


Line 1123: 	break;
Nitpick: All of the breaks in this switch statement are indented oddly. Typically in Asterisk, we indent them the same amount as the code within the case.


Line 1132: 		/* Sort can only work on something with a full or partial key. */
I don't understand this comment.


Line 1152: 		dev_iter = ao2_t_callback(hintdevices, OBJ_SEARCH_KEY | OBJ_MULTIPLE,
         : 			hintdevice_cmp_multiple, device, "find devices in container");
Since the goal of this is to remove items from the hintdevices container, would it be more efficient to use a callback that returns CMP_MATCH if

1) The hintdevice's hintdevice string matches the current hint's device and
2) The hintdevice's hint matches the current hint

and you pass OBJ_UNLINK to ao2_t_callback()?

It seems like you'd save yourself some extra iteration this way.


Line 1154: 		if (dev_iter) {
         : 			struct ast_hintdevice *hintdevice;
         : 			for (; (hintdevice = ao2_iterator_next(dev_iter)); ao2_ref(hintdevice, -1)) {
         : 				/* Only remove those hintdevices for our hint */
         : 				if (hintdevice->hint == hint) {
         : 					ao2_unlink(hintdevices, hintdevice);
         : 				}
         : 			}
         : 		}
You need to destroy dev_iter.


Line 1163: 		AST_VECTOR_REMOVE_UNORDERED(&hint->devices, i);
         : 		ast_free(device);
         : 		AST_VECTOR_INSERT(&hint->devices, i, NULL);
I'm guessing the idea behind these two vector invocations is to get rid of the current item in the vector but maintain the items after i so that the size of the vector remains the same and the for loop doesn't break unexpectedly.

Since the goal of this loop is to remove all items from the vector, this seems a bit odd. You could do something more like this:

    while (AST_VECTOR_SIZE(&hint->devices) > 0) {
        char *device = AST_VECTOR_GET(&hint->devices, 0);
        ...do stuff...
        AST_RECTOR_REMOVE_UNORDERED(&hint->devices, 0);
    }

This way, you should never end up with NULL devices in the vector, and the size of the vector will accurately reflect the devices inside. I also just think the code is more clear and doesn't rely on the underlying implementation details of the vector.


-- 
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-HasComments: Yes