On 11/20/2014 07:31 AM, Dave WOOLLEY wrote:
Whilst looking at the performance of the search for an available queue
member I’ve come across code that handles the ringinuse flag that
seems to count busy an inuse as the same, that doesn’t make sense to
me. I’ve also come across very long standing code that means
ringinuse disables state checks entirely, allowing the code ot fall
through to the, very expensive, compare_weights code. Finally, the
compare_weights code seems to waste a lot of time trying to match a
member in queues that don’t have higher weights.
Looking at the current trunk version, revision 325483 introduced
additional “case” lines into, what is now, is_member_available, which
treat busy and inuse the same:
2310 case AST_DEVICE_INUSE:
2311 case AST_DEVICE_BUSY:
2312 case AST_DEVICE_RINGING:
2313 case AST_DEVICE_RINGINUSE:
2314 case AST_DEVICE_ONHOLD:
2315 if (!mem->ringinuse) {
2316 break;
2317 }
Whilst this postdates the code we are using, so doesn’t directly
affect us, this seems wrong to me; I thought the whole subtlety behind
calling it ringinuse, not ringbusy, is that there is a difference
between busy and in use. Could someone explain?
Yeah that looks wrong to me. When I was last involved in writing queue
behavior code, the queue was supposed to never attempt to send calls to
busy members, ever. The ringinuse option should only have an effect on
members who are in use. It should have no effect on members who are
busy. It's possible that the code moved in its current direction
"organically" through community patches, or it could just be a mistake.
Next, in can_ring_entry, ringinuse set completely bypasses the device
state check:
4169 if (!call->member->ringinuse &&
!member_status_available(call->member->status)) {
4170 ast_debug(1, "%s not available, can't
receive call\n", call->interface);
4171 return 0;
4172 }
The consequence of this is that, if ringinuse is set, even if the
member is unavailable (e.g. agent logged out), or definitively busy,
the code falls through to:
4182 if (use_weight && compare_weight(qe->parent, call->member)) {
4183 ast_debug(1, "Priority queue delaying call
to %s:%s\n",
4184 qe->parent->name, call->interface);
4185 return 0;
4186 }
compare_weight is very expensive, because it iterates over all queues
and all members of them. Also, at least in the version we are using,
although I haven’t checked this in trunk, there is a lock held on the
queue of interest and locks are taken, in enumeration sequence, on all
the other queues. This looks to me that it will cause serialisation
of access to that function.
So, finally, looking at compare_weight, this line:
4080 if (q->weight > rq->weight && q->count >= num_available_members(q)) {
Is run after this line:
4078 if ((mem = ao2_find(q->members, member, OBJ_POINTER))) {
Whilst the second term is expensive, involving a scan of all the
members, the first term is cheap. In our case, there are lots of
members in common between lots of queues, a lot of which are not at
high priority. It seems to me that doing the cheap test before the
search for the member, would quickly eliminate queues that could never
pre-empt the agent.. Even if ao2_find hashes, it is still going to be
expensive compared with a compare of two, one level indirect, scalars.
This is a case where patches are certainly welcome. The q->members
container should be a hash table for all queue strategies aside from the
linear strategy. That requires the use of a linked list. Assuming the
worst case at all times, you're right that the search for a specific
member is going to be more expensive than the check of the queue weight.
This could probably be refactored to compare weights first, then find
the specified member, and then finally check the number of available
members in the queue.
One other observation, is that compare_weight doesn’t account for
members that are unavailable to the conflicting queue because there is
an even higher priority queue. NB the member in question need not be
the same member as is being considered for the call. (Without a more
efficient algorithm, recursing the check would add additional
multipliers to the computational complexity, so could well be unworkable.)
BTS Holdings PLC - Registered office: BTS House, Manor Road,
Wallington, SM6 0DD - Registered in England: 1517630
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
