Mark Kettenis <[email protected]> writes: > I tried making the FlushClient() implementation a bit smarter by using > a circular buffer to avoid the memmove'ing. Unfortunately that makes > resizing the output buffer more painful and introduces some additional > memcpy'ing there.
I don't think there's actually any more memcpy going on -- you've
replaced a realloc with a malloc followed by memcpy, which should be the
same cost.
I've merged your patch along with Daniel's and my output reservation
idea into a single sequence so I could test the effect of each one.
1: glamor-server my glamor-server branch
2: one-buffer + Daniel's single buffer for GetImage patch
3: ring-buffer + Mark's ring output buffer patch
4: output-reservation + Keith's output reservation patch
1 2 3
4 Operation
------------ ------------------------- -------------------------
------------------------- -------------------------
486.0 583.0 ( 1.200) 603.0 ( 1.241)
619.0 ( 1.274) GetImage 500x500 square
All three have positive effects (oddly), with the bulk of the
improvement going to not calling the DDX in tiny chunks. I assume that
the ring output buffer helps by not calling memmove after every network
write (yikes!), while my output reservation patch has a smaller effect
by not copying the data twice.
This is with my glamor driver, which doesn't have any extra cost for
each GetImage as it's just calling glReadPixels.
Here's the patch that I made:
From 64c3c1ef299d86d4a5682b2c366ea4a4131e6902 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Mon, 31 Mar 2014 22:53:27 -0700 Subject: [PATCH] Add client output buffer reservation API for GetImage Instead of allocating a temporary image buffer and then copying from there to the OS output buffer, create a new OS API to reserve space in the OS output buffer directly and get the driver to write directly into that. Signed-off-by: Keith Packard <[email protected]> --- dix/dispatch.c | 13 +++--- include/os.h | 4 ++ os/io.c | 134 +++++++++++++++++++++++++++++++++++++++++++++------------ 3 files changed, 119 insertions(+), 32 deletions(-) diff --git a/dix/dispatch.c b/dix/dispatch.c index a0b1bc0..ab05c91 100644 --- a/dix/dispatch.c +++ b/dix/dispatch.c @@ -2072,9 +2072,13 @@ DoGetImage(ClientPtr client, int format, Drawable drawable, } xgi.length = bytes_to_int32(length); - if (!(pBuf = calloc(1, length))) + + pBuf = ReserveClientOutputSpace(client, length + sizeof (xGetImageReply)); + if (length != 0 && !pBuf) return BadAlloc; - WriteReplyToClient(client, sizeof(xGetImageReply), &xgi); + + memcpy(pBuf, &xgi, sizeof (xGetImageReply)); + pBuf += sizeof (xGetImageReply); if (pDraw->type == DRAWABLE_WINDOW) { pVisibleRegion = NotClippedByChildren((WindowPtr) pDraw); @@ -2102,7 +2106,6 @@ DoGetImage(ClientPtr client, int format, Drawable drawable, ReformatImage(pBuf, length, BitsPerPixel(pDraw->depth), ClientOrder(client)); - WriteToClient(client, length, pBuf); } else { /* XYPixmap */ int plane_size = height * widthBytesLine; @@ -2122,13 +2125,13 @@ DoGetImage(ClientPtr client, int format, Drawable drawable, as we do NOT byte swap */ ReformatImage(pBuf, plane_size, 1, ClientOrder(client)); - WriteToClient(client, plane_size, pBuf); + pBuf += plane_size; } } } if (pVisibleRegion) RegionDestroy(pVisibleRegion); - free(pBuf); + CommitClientOutputSpace (client, sizeof (xGetImageReply) + length); return Success; } diff --git a/include/os.h b/include/os.h index d26e399..447fbdc 100644 --- a/include/os.h +++ b/include/os.h @@ -119,6 +119,10 @@ extern _X_EXPORT void SetCriticalOutputPending(void); extern _X_EXPORT int WriteToClient(ClientPtr /*who */ , int /*count */ , const void * /*buf */ ); +extern _X_EXPORT void *ReserveClientOutputSpace(ClientPtr who, int count); + +extern _X_EXPORT int CommitClientOutputSpace(ClientPtr who, int count); + extern _X_EXPORT void ResetOsBuffers(void); extern _X_EXPORT void InitConnectionLimits(void); diff --git a/os/io.c b/os/io.c index 1a63d37..0d37746 100644 --- a/os/io.c +++ b/os/io.c @@ -867,7 +867,112 @@ WriteToClient(ClientPtr who, int count, const void *__buf) return count; } - /******************** +static Bool +ResizeOutputBuffer(ClientPtr who, OsCommPtr oc, int size) +{ + ConnectionOutputPtr oco = oc->output; + uint8_t *obuf; + + obuf = malloc (size); + if (!obuf) { + _XSERVTransDisconnect(oc->trans_conn); + _XSERVTransClose(oc->trans_conn); + oc->trans_conn = NULL; + MarkClientException(who); + oco->start = 0; + oco->count = 0; + return FALSE; + } + if (oco->start + oco->count > oco->size) { + memcpy(obuf, (char *)oco->buf + oco->start, oco->size - oco->start); + memcpy((char *) obuf + (oco->size - oco->start), oco->buf, oco->count - (oco->size - oco->start)); + oco->start = 0; + } else { + memcpy((char *) obuf, (char *) oco->buf + oco->start, oco->count); + oco->start = 0; + } + oco->size = size; + free(oco->buf); + oco->buf = obuf; + return TRUE; +} + +/******************** + * ReserveClientOutputSpace + * Allocates space in the client output buffer, + * returning a pointer to the desired space or NULL + * on allocation failure + * + **********************/ + +void * +ReserveClientOutputSpace(ClientPtr who, int count) +{ + OsCommPtr oc; + ConnectionOutputPtr oco; + + if (!count || !who || who == serverClient || who->clientGone) + return NULL; + oc = who->osPrivate; + oco = oc->output; + + if (!oco) { + if ((oco = FreeOutputs)) { + FreeOutputs = oco->next; + } + else if (!(oco = AllocateOutputBuffer())) { + if (oc->trans_conn) { + _XSERVTransDisconnect(oc->trans_conn); + _XSERVTransClose(oc->trans_conn); + oc->trans_conn = NULL; + } + MarkClientException(who); + return NULL; + } + oc->output = oco; + } + + count += padding_for_int32(count); + + if (oco->count + count > oco->size) { + if (!ResizeOutputBuffer(who, oc, oco->count + count + BUFSIZE)) + return NULL; + } + + return oco->buf + oco->count + oco->start; +} + +/******************** + * CommitClientOutputSpace + * Marches the buffer pointers past the + * allocate amount of space + * + **********************/ + +int +CommitClientOutputSpace(ClientPtr who, int count) +{ + OsCommPtr oc; + ConnectionOutputPtr oco; + + if (!count || !who || who == serverClient || who->clientGone) + return -1; + oc = who->osPrivate; + oco = oc->output; + + if (!oco) + return -1; + + count += padding_for_int32(count); + oco->count += count; + + if (FlushCallback) + CallCallbacks(&FlushCallback, NULL); + + return FlushClient(who, oc, NULL, 0); +} + +/******************** * FlushClient() * If the client isn't keeping up with us, then we try to continue * buffering the data and set the apropriate bit in ClientsWritable @@ -978,33 +1083,8 @@ FlushClient(ClientPtr who, OsCommPtr oc, const void *__extraBuf, int extraCount) } if (notWritten > oco->size) { - unsigned char *obuf; - - if (oco->start > 0) - fprintf(stderr, "x"); - else - fprintf(stderr, "y"); - obuf = malloc(notWritten + BUFSIZE); - if (!obuf) { - _XSERVTransDisconnect(oc->trans_conn); - _XSERVTransClose(oc->trans_conn); - oc->trans_conn = NULL; - MarkClientException(who); - oco->start = 0; - oco->count = 0; + if (!ResizeOutputBuffer(who, oc, notWritten + BUFSIZE)) return -1; - } - if (oco->start + oco->count > oco->size) { - memcpy(obuf, (char *)oco->buf + oco->start, oco->size - oco->start); - memcpy((char *) obuf + (oco->size - oco->start), oco->buf, oco->count - (oco->size - oco->start)); - oco->start = 0; - } else { - memcpy((char *) obuf, (char *) oco->buf + oco->start, oco->count); - oco->start = 0; - } - oco->size = notWritten + BUFSIZE; - free(oco->buf); - oco->buf = obuf; } /* If the amount written extended into the padBuffer, then the -- 1.9.0
-- [email protected]
pgpx8dy88vWqn.pgp
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
