From: Antonin Houska <ah@cybertec.at>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
cc: pgsql-hackers@postgresql.org
Subject: Re: asynchronous execution
In-reply-to: <20170704.130846.251038264.horiguchi.kyotaro@lab.ntt.co.jp>
References: <20170522.142253.40733906.horiguchi.kyotaro@lab.ntt.co.jp>
 <20170622.141720.190958545.horiguchi.kyotaro@lab.ntt.co.jp>
 <392.1498674144@localhost>
 <20170704.130846.251038264.horiguchi.kyotaro@lab.ntt.co.jp>
Comments: In-reply-to Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
   message dated "Tue, 04 Jul 2017 13:08:46 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Tue, 11 Jul 2017 10:28:51 +0200
Message-ID: <6448.1499761731@localhost>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:

> > Just one idea that I had while reading the code.
> >=20
> > In ExecAsyncEventLoop you iterate estate->es_pending_async, then move t=
he
> > complete requests to the end and finaly adjust estate->es_num_pending_a=
sync so
> > that the array no longer contains the complete requests. I think the po=
int is
> > that then you can add new requests to the end of the array.
> >=20
> > I wonder if a set (Bitmapset) of incomplete requests would make the cod=
e 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 t=
he
> > requests subject to ExecAsyncNotify etc, then the compaction of
> > estate->es_pending_async wouldn't be necessary.
> >=20
> > 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 com=
pleted
> > request).
> >=20
> > And finally, item would be removed from the set at the moment the reque=
st
> > state is being set to ASYNCREQ_COMPLETE.
>=20
> 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.
>=20
> 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 th=
is
make sense?


		/* If any node completed, compact the array. */
		if (any_node_done)
		{
			int		hidx =3D 0,
					tidx;

			/*
			 * Swap all non-yet-completed items to the start of the array.
			 * Keep them in the same order.
			 */
			for (tidx =3D 0; tidx < estate->es_num_pending_async; ++tidx)
			{
				PendingAsyncRequest *tail =3D estate->es_pending_async[tidx];

				Assert(tail->state !=3D ASYNCREQ_CALLBACK_PENDING);

				if (tail->state =3D=3D 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 =3D 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 =3D=3D ASYNCREQ_COMPLETE);

					estate->es_pending_async[tidx] =3D head;
					estate->es_pending_async[hidx] =3D tail;
				}

				++hidx;
			}

			estate->es_num_pending_async =3D hidx;
		}

And besides that, I think it'd be more intuitive if the meaning of "head" a=
nd
"tail" was reversed: if the array is iterated from lower to higher position=
s,
then I'd consider head to be at higher position, not tail.

--=20
Antonin Houska Cybertec Sch=C3=B6nig & Sch=C3=B6nig GmbH Gr=C3=B6hrm=C3=BCh=
lgasse 26 A-2700 Wiener
Neustadt Web: http://www.postgresql-support.de, http://www.cybertec.at


--=20
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers