Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> wrote:
> > Just one idea that I had while reading the code.
> >
> > In ExecAsyncEventLoop you iterate estate->es_pending_async, then move the
> > complete requests to the end and finaly adjust estate->es_num_pending_async
> > so
> > that the array no longer contains the complete requests. I think the point
> > is
> > that then you can add new requests to the end of the array.
> >
> > I wonder if a set (Bitmapset) of incomplete requests would make the code
> > more
> > efficient. The set would contain position of each incomplete request in
> > estate->es_num_pending_async (I think it's the myindex field of
> > PendingAsyncRequest). If ExecAsyncEventLoop used this set to retrieve the
> > requests subject to ExecAsyncNotify etc, then the compaction of
> > estate->es_pending_async wouldn't be necessary.
> >
> > ExecAsyncRequest would use the set to look for space for new requests by
> > iterating it and trying to find the first gap (which corresponds to
> > completed
> > request).
> >
> > And finally, item would be removed from the set at the moment the request
> > state is being set to ASYNCREQ_COMPLETE.
>
> Effectively it is a waiting-queue followed by a
> completed-list. The point of the compaction is keeping the order
> of waiting or not-yet-completed requests, which is crucial to
> avoid kind-a precedence inversion. We cannot keep the order by
> using bitmapset in such way.
> The current code waits all waiters at once and processes all
> fired events at once. The order in the waiting-queue is
> inessential in the case. On the other hand I suppoese waiting on
> several-tens to near-hundred remote hosts is in a realistic
> target range. Keeping the order could be crucial if we process a
> part of the queue at once in the case.
>
> Putting siginificance on the deviation of response time of
> remotes, process-all-at-once is effective. In turn we should
> consider the effectiveness of the lifecycle of the larger wait
> event set.
ok, I missed the fact that the order of es_pending_async entries is
important. I think this is worth adding a comment.
Actually the reason I thought of simplification was that I noticed small
inefficiency in the way you do the compaction. In particular, I think it's not
always necessary to swap the tail and head entries. Would something like this
make sense?
/* If any node completed, compact the array. */
if (any_node_done)
{
int hidx = 0,
tidx;
/*
* Swap all non-yet-completed items to the start of the
array.
* Keep them in the same order.
*/
for (tidx = 0; tidx < estate->es_num_pending_async;
++tidx)
{
PendingAsyncRequest *tail =
estate->es_pending_async[tidx];
Assert(tail->state !=
ASYNCREQ_CALLBACK_PENDING);
if (tail->state == ASYNCREQ_COMPLETE)
continue;
/*
* If the array starts with one or more
incomplete requests,
* both head and tail point at the same item,
so there's no
* point in swapping.
*/
if (tidx > hidx)
{
PendingAsyncRequest *head =
estate->es_pending_async[hidx];
/*
* Once the tail got ahead, it should
only leave
* ASYNCREQ_COMPLETE behind. Only those
can then be seen
* by head.
*/
Assert(head->state ==
ASYNCREQ_COMPLETE);
estate->es_pending_async[tidx] = head;
estate->es_pending_async[hidx] = tail;
}
++hidx;
}
estate->es_num_pending_async = hidx;
}
And besides that, I think it'd be more intuitive if the meaning of "head" and
"tail" was reversed: if the array is iterated from lower to higher positions,
then I'd consider head to be at higher position, not tail.
--
Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener
Neustadt Web: http://www.postgresql-support.de, http://www.cybertec.at
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
