[PATCH] Reduce realloc syscall overhead for ft2232 and bitbang

[Carl-Daniel Hailfinger]

Reduce realloc syscall overhead for FT2232 and bitbang.

FT2232 ran realloc() for every executed command. Start with a big enough
buffer and don't touch buffer size unless it needs to grow.
Bitbang was slightly better: It only ran realloc() if buffer size
changed. Still, the solution above improves performance and reliability.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-realloc_overhead/bitbang_spi.c
===================================================================
--- flashrom-realloc_overhead/bitbang_spi.c	(Revision 777)
+++ flashrom-realloc_overhead/bitbang_spi.c	(Arbeitskopie)
@@ -87,14 +87,16 @@
    static unsigned char *bufout = NULL;
    static unsigned char *bufin = NULL;
    static int oldbufsize = 0;
-	int bufsize = max(writecnt + readcnt, 260);
+	int bufsize;
    int i;

/* Arbitrary size limitation here. We're only constrained by memory. */
    if (writecnt > 65536 || readcnt > 65536)
    	return SPI_INVALID_LENGTH;

-	if (bufsize != oldbufsize) {
+	bufsize = max(writecnt + readcnt, 260);
+	/* Never shrink. realloc() calls are expensive. */
+	if (bufsize > oldbufsize) {
    	bufout = realloc(bufout, bufsize);
    	if (!bufout) {
    		fprintf(stderr, "Out of memory!\n");
@@ -109,6 +111,7 @@
    			free(bufout);
    		exit(1);
    	}
+		oldbufsize = bufsize;
    }
    	
    memcpy(bufout, writearr, writecnt);
Index: flashrom-realloc_overhead/ft2232_spi.c
===================================================================
--- flashrom-realloc_overhead/ft2232_spi.c	(Revision 777)
+++ flashrom-realloc_overhead/ft2232_spi.c	(Arbeitskopie)
@@ -200,14 +200,22 @@
    static unsigned char *buf = NULL;
    /* failed is special. We use bitwise ops, but it is essentially bool. */
    int i = 0, ret = 0, failed = 0;
+	int bufsize;
+	static int oldbufsize = 0;

if (writecnt > 65536 || readcnt > 65536)
    	return SPI_INVALID_LENGTH;

-	buf = realloc(buf, writecnt + readcnt + 100);
-	if (!buf) {
-		fprintf(stderr, "Out of memory!\n");
-		exit(1); // -1
+	/* buf is not used for the response from the chip. */
+	bufsize = max(writecnt + 9, 260);
+	/* Never shrink. realloc() calls are expensive. */
+	if (bufsize > oldbufsize) {
+		buf = realloc(buf, bufsize);
+		if (!buf) {
+			fprintf(stderr, "Out of memory!\n");
+			exit(1);
+		}
+		oldbufsize = bufsize;
    }

/*

[PATCH] Reduce realloc syscall overhead for ft2232 and bitbang

[Paul Fox]

carl-daniel wrote:
> Reduce realloc syscall overhead for FT2232 and bitbang.
>
> FT2232 ran realloc() for every executed command. Start with a big enough
> buffer and don't touch buffer size unless it needs to grow.
> Bitbang was slightly better: It only ran realloc() if buffer size
> changed. Still, the solution above improves performance and reliability.

this is fine, but i'm curious -- did you measure a performance
change, or is this "by inspection"?  it's never been my
impression that historically realloc would be slow, unless the
buffer is growing -- in which case there's no choice.  and my perhaps
mistaken assumption was that realloc() would usually not shrink a
buffer.

paul
> Signed-off-by: Carl-Daniel Hailfinger
> Index: flashrom-realloc_overhead/bitbang_spi.c
>
> --- flashrom-realloc_overhead/bitbang_spi.c	(Revision 777)
> +++ flashrom-realloc_overhead/bitbang_spi.c	(Arbeitskopie)
> @@ -87,14 +87,16 @@
>    static unsigned char *bufout = NULL;
>    static unsigned char *bufin = NULL;
>    static int oldbufsize = 0;
> int bufsize = max(writecnt + readcnt, 260);
>
> int bufsize;
> int i;
>
> /* Arbitrary size limitation here. We're only constrained by memory. */
> if (writecnt > 65536 || readcnt > 65536)
> return SPI_INVALID_LENGTH;
> if (bufsize != oldbufsize) {
> bufsize = max(writecnt + readcnt, 260);
> /* Never shrink. realloc() calls are expensive. */
> if (bufsize > oldbufsize) {
> bufout = realloc(bufout, bufsize);
> if (!bufout) {
> 	fprintf(stderr, "Out of memory!\n");
>
> @@ -109,6 +111,7 @@
>    			free(bufout);
>    		exit(1);
>    	}
> oldbufsize = bufsize;
> }
>
> memcpy(bufout, writearr, writecnt);
> Index: flashrom-realloc_overhead/ft2232_spi.c
>
> --- flashrom-realloc_overhead/ft2232_spi.c	(Revision 777)
> +++ flashrom-realloc_overhead/ft2232_spi.c	(Arbeitskopie)
> @@ -200,14 +200,22 @@
>    static unsigned char *buf = NULL;
>    /* failed is special. We use bitwise ops, but it is essentially bool. */
>    int i = 0, ret = 0, failed = 0;
>
> int bufsize;
>
> static int oldbufsize = 0;
>
> if (writecnt > 65536 || readcnt > 65536)
> return SPI_INVALID_LENGTH;
> buf = realloc(buf, writecnt + readcnt + 100);
> if (!buf) {
> fprintf(stderr, "Out of memory!\n");
> exit(1); // -1
>
> /* buf is not used for the response from the chip. */
>
> bufsize = max(writecnt + 9, 260);
>
> /* Never shrink. realloc() calls are expensive. */
>
> if (bufsize > oldbufsize) {
> buf = realloc(buf, bufsize);
> if (!buf) {
> 	fprintf(stderr, "Out of memory!\n");
> 	exit(1);
> }
> oldbufsize = bufsize;
>
> }
>
> /*

=---------------------
 paul fox, [e-mail] (arlington, ma, where it's 47.1 degrees)

[PATCH] Reduce realloc syscall overhead for ft2232 and bitbang

[Sean Nelson]

On 11/25/2009 7:31 AM, Paul Fox wrote:
> carl-daniel wrote:
> > Reduce realloc syscall overhead for FT2232 and bitbang.
> >
> > FT2232 ran realloc() for every executed command. Start with a big enough
> >  buffer and don't touch buffer size unless it needs to grow.
> >  Bitbang was slightly better: It only ran realloc() if buffer size
> >  changed. Still, the solution above improves performance and reliability.
>
> this is fine, but i'm curious -- did you measure a performance
> change, or is this "by inspection"?  it's never been my
> impression that historically realloc would be slow, unless the
> buffer is growing -- in which case there's no choice.  and my perhaps
> mistaken assumption was that realloc() would usually not shrink a
> buffer.
>
> paul
> > Signed-off-by: Carl-Daniel Hailfinger
> > Index: flashrom-realloc_overhead/bitbang_spi.c
> >
> > --- flashrom-realloc_overhead/bitbang_spi.c	(Revision 777)
> >  +++ flashrom-realloc_overhead/bitbang_spi.c	(Arbeitskopie)
> >  @@ -87,14 +87,16 @@
> >    static unsigned char *bufout = NULL;
> >    static unsigned char *bufin = NULL;
> >    static int oldbufsize = 0;
> > int bufsize = max(writecnt + readcnt, 260);
> >
> > int bufsize;
> > int i;
> >
> > /* Arbitrary size limitation here. We're only constrained by memory. */
> > if (writecnt>  65536 || readcnt>  65536)
> > return SPI_INVALID_LENGTH;
> > if (bufsize != oldbufsize) {
> > bufsize = max(writecnt + readcnt, 260);
> > /* Never shrink. realloc() calls are expensive. */
> > if (bufsize>  oldbufsize) {
> > bufout = realloc(bufout, bufsize);
> > if (!bufout) {
> > 	fprintf(stderr, "Out of memory!\n");
> >
> > @@ -109,6 +111,7 @@
> >    			free(bufout);
> >    		exit(1);
> >    	}
> > oldbufsize = bufsize;
> > }
> >
> > memcpy(bufout, writearr, writecnt);
> > Index: flashrom-realloc_overhead/ft2232_spi.c
> >
> > --- flashrom-realloc_overhead/ft2232_spi.c	(Revision 777)
> >  +++ flashrom-realloc_overhead/ft2232_spi.c	(Arbeitskopie)
> >  @@ -200,14 +200,22 @@
> >    static unsigned char *buf = NULL;
> >    /* failed is special. We use bitwise ops, but it is essentially bool. */
> >    int i = 0, ret = 0, failed = 0;
> >
> > int bufsize;
> >
> > static int oldbufsize = 0;
> >
> > if (writecnt>  65536 || readcnt>  65536)
> > return SPI_INVALID_LENGTH;
> > buf = realloc(buf, writecnt + readcnt + 100);
> > if (!buf) {
> > fprintf(stderr, "Out of memory!\n");
> > exit(1); // -1
> >
> > /* buf is not used for the response from the chip. */
> >
> > bufsize = max(writecnt + 9, 260);
> >
> > /* Never shrink. realloc() calls are expensive. */
> >
> > if (bufsize>  oldbufsize) {
> > buf = realloc(buf, bufsize);
> > if (!buf) {
> > 	fprintf(stderr, "Out of memory!\n");
> > 	exit(1);
> > }
> > oldbufsize = bufsize;
> >
> > }
> >
> > /*
>
> =---------------------
>   paul fox, [e-mail] (arlington, ma, where it's 47.1 degrees)
>
> flashrom mailing list
> [e-mail]
> http://www.flashrom.org/mailman/listinfo/flashrom

I always assume any memory resizing calls are expensive due to the 
content checking; if shrinking check that the space isn't used, if 
expanding check that the adjacent memory segments aren't used, or 
searching for a space that isn't used.

The patch looks fine and I see no problems so:
Acked-by: Sean Nelson

[PATCH] Reduce realloc syscall overhead for ft2232 and bitbang

[Carl-Daniel Hailfinger]

On 25.11.2009 16:59, Sean Nelson wrote:
> Acked-by: Sean Nelson

Thanks, committed in r780 with one minor change: Default alloc size for
FT2232 is now 269 bytes instead of 260 bytes (260 bytes is usual max SPI
communication length, 9 bytes is default total FT2232 command length).

Regards,
Carl-Daniel

[PATCH] Reduce realloc syscall overhead for ft2232 and bitbang

[Carl-Daniel Hailfinger]

On 25.11.2009 16:31, Paul Fox wrote:
> carl-daniel wrote:
> > Reduce realloc syscall overhead for FT2232 and bitbang.
> >
> > FT2232 ran realloc() for every executed command. Start with a big enough
> > buffer and don't touch buffer size unless it needs to grow.
> > Bitbang was slightly better: It only ran realloc() if buffer size
> > changed. Still, the solution above improves performance and reliability.
>
> this is fine, but i'm curious -- did you measure a performance
> change, or is this "by inspection"?  it's never been my
> impression that historically realloc would be slow, unless the
> buffer is growing -- in which case there's no choice.  and my perhaps
> mistaken assumption was that realloc() would usually not shrink a
> buffer.

AFAIK shrinking happens if allocations differ by an order of magnitude
and at least >n size of bytes (I don't have n handy right now).

Given that FT2232 varies buffer sizes from 10 bytes (min) to 269 bytes
(max) in continuous succession, I think at least some libraries will
shrink and expand the buffer too often. If that happens, every second
call to realloc will be growing the buffer. Some people are compiling
flashrom against dietlibc and other small embedded libc variants, and
those usually are performing tighter memory management.

Thanks for the review. While answering I noticed that I should have
picked 269 instead of 260 as one of the parameters for the max() call.

Regards,
Carl-Daniel