Subject: Re: bug#20029: 'yes' surprisingly slow

From: Pádraig Brady <[email-masked]>
Date: Mon, 09 Mar 2015 22:14:57 +0000

On 09/03/15 20:02, Eric Blake wrote:
> On 03/09/2015 01:47 PM, Pádraig Brady wrote:
> 
>>
>> Note this change also ensures that yes will only write complete lines
>> for lines softer than BUFSIZ.
> 
> s/softer/smaller/
> 
>>
>> * src/yes.c (main): Build up a BUFSIZ buffer of lines,
>> and output that, rather than having stdio process each item.
>> * tests/misc/yes.sh: Add a new test for various buffer sizes.
>> * tests/local.mk: Reference the new test.
>> Fixes http://bugs.gnu.org/20029
>> ---
>>  src/yes.c         | 43 +++++++++++++++++++++++++++++++++++++++++--
>>  tests/local.mk    |  1 +
>>  tests/misc/yes.sh | 28 ++++++++++++++++++++++++++++
>>  3 files changed, 70 insertions(+), 2 deletions(-)
>>  create mode 100755 tests/misc/yes.sh
>>
>> diff --git a/src/yes.c b/src/yes.c
>> index b35b13f..91dea11 100644
>> --- a/src/yes.c
>> +++ b/src/yes.c
>> @@ -58,6 +58,10 @@ Repeatedly output a line with all specified STRING(s), or 'y'.\n\
>>  int
>>  main (int argc, char **argv)
>>  {
>> +  char buf[BUFSIZ];
> 
> Do you really want this stack-allocated?  BUFSIZ can be larger than a
> page, which can then interfere with stack overflow detection.

Well we do such stack buffers elsewhere:

$ git grep char.*\\[BUFSIZ\\]
src/head.c:  char buf[BUFSIZ];
src/head.c:    char buffer[BUFSIZ];
src/head.c:  char buffer[BUFSIZ];
src/head.c:  char buffer[BUFSIZ];
src/head.c:  char buffer[BUFSIZ];
src/ls.c:  char smallbuf[BUFSIZ];
src/od.c:              char buf[BUFSIZ];
src/tail.c:      char buffer[BUFSIZ];
src/tail.c:  char buffer[BUFSIZ];
src/tail.c:    char buffer[BUFSIZ];
src/tail.c:    char buffer[BUFSIZ];
src/tail.c:  char buffer[BUFSIZ];
src/tail.c:      char buffer[BUFSIZ];
src/tee.c:  char buffer[BUFSIZ];
src/tr.c:static char io_buf[BUFSIZ];
src/uptime.c:      char buf[BUFSIZ];
src/yes.c:  char buf[BUFSIZ];

We would probably change them all if this was thought to be a problem.

>> +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
>> +print_ver_ yes
>> +
>> +for size in 1 4095 4096 8191 8192 16383 16384; do
> 
> Should you also test 4097 8193 and 16385 (that is, a likely
> 1-more-than-BUFSIZ in the mix)?

The 1 more is implicit with the \n added by yes(1).

thanks for the review!
Pádraig