[SCM] strace branch, master, updated. v4.6-127-g8778bff

[Denys Vlasenko]

On Thu, 2011-09-01 at 00:18 +0400, Dmitry V. Levin wrote:
> On Wed, Aug 31, 2011 at 10:25:34AM +0000, Denys Vlasenko wrote:
> > commit 8778bffdd25dca050b3aa2a7a7e05bc8a63a6665
> > Author: Denys Vlasenko <dvlasenk at redhat.com>
> > Date:   Wed Aug 31 12:22:56 2011 +0200
> > 
> >     Optimize string_quote() for speed
> >     
> >     * util.c (string_quote): Speed up check for terminating NUL.
> >     Replace strintf() with open-coded binary to hex/oct conversions -
> >     we potentially do them for every single byte, need to be fast.
> 
> The change is mostly OK, but
> 
> > --- a/util.c
> > +++ b/util.c
> > @@ -440,8 +440,15 @@ string_quote(const char *instr, char *outstr, int len, int size)
> >  {
> >  	const unsigned char *ustr = (const unsigned char *) instr;
> >  	char *s = outstr;
> > -	int usehex = 0, c, i;
> > +	int usehex, c, i, eol;
> >  
> > +	eol = 0x100; /* this can never match a char */
> > +	if (len < 0) {
> > +		size--;
> 
> I spent some time here to ensure that the assumed assertion "size > 0"
> is always true.

It does not have to. If size will become -1, the code will still work
correctly.

>   Well, it seems to be true indeed, but the code now
> looks more fragile than before.
> 
> > @@ -471,27 +473,19 @@ string_quote(const char *instr, char *outstr, int len, int size)
> >  		for (i = 0; i < size; ++i) {
> >  			c = ustr[i];
> >  			/* Check for NUL-terminated string. */
> > -			if (len < 0) {
> > -				if (c == '\0')
> > -					break;
> > -				/* Quote at most size - 1 bytes. */
> > -				if (i == size - 1)
> > -					continue;
> > -			}
> > -			sprintf(s, "\\x%02x", c);
> > -			s += 4;
> > +			if (c == eol)
> > +				goto asciz_ended;
> 
> I'd rather replaced "goto asciz_ended" with "break" here

"break" will jump to code which checks (len < 0 && ustr[i] == '\0').
What's the point in spending time checking that if we already know
that it's true? Especially that "goto" and "break" on machine
code level are both jumps, we don't win anything by using "break".


> > @@ -536,8 +542,21 @@ string_quote(const char *instr, char *outstr, int len, int size)
> >  	*s++ = '\"';
> >  	*s = '\0';
> >  
> > -	/* Return nonzero if the string was unterminated.  */
> > -	return i == size;
> > +	/* Return zero if we printed entire ASCIZ string (didn't truncate it) */
> > +	if (len < 0 && ustr[i] == '\0') {
> 
> and replaced this condition with
> 
> 	if (len < 0 && (i < size || ustr[i] == '\0'))
> 
> so the could would look more compact and readable.

Added "i < size" check is usually two instructions. The code your
proposal removes:

*s++ = '\"';
*s = '\0';
return 0;

is four instructions. IOW: I propose to trade 2 insn shorter code
for an unnecessary (len < 0 && (i < size || ustr[i] == '\0')) check
when NUL is encountered.

I like small code (I consider myself "-Os guy", not "-O2 guy"), but in
such an often called function as string_quote(),
I think we'd rather be faster than smaller.

-- 
vda