Re: [PATCH v6 2/2] refs.c: SSE4.2 optimizations for check_refname_component

From: David Turner <dturner@twopensource.com>
Date: Jun 4, 2014, 21:14 UTC
Message-ID: <1401916476.18134.165.camel@stross>
In-Reply-To: <538ED2F1.9030003@web.de>

On Wed, 2014-06-04 at 10:04 +0200, Torsten Bögershausen wrote: [snip discussion of compiler flags; I'll look into a cpuid approach]

Show 21 quoted lines

> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -668,6 +668,28 @@ void git_qsort(void *base, size_t nmemb, size_t size,
> >  #endif
> >  #endif
> >  
> > +#ifndef NO_SSE42
> > +#include <nmmintrin.h>
> > +/*
> > + * Clang ships with a version of nmmintrin.h that's incomplete; if
> > + * necessary, we define the constants that we're going to use.
> > + */
> > +#ifndef _SIDD_UBYTE_OPS
> > +#define _SIDD_UBYTE_OPS                 0x00
> > +#define _SIDD_CMP_EQUAL_ANY             0x00
> > +#define _SIDD_CMP_RANGES                0x04
> > +#define _SIDD_CMP_EQUAL_ORDERED         0x0c
> > +#define _SIDD_NEGATIVE_POLARITY         0x10
> > +#endif
> Why do this defines end up in git-compat-util.h when they are needed by one file?
> (see even below)

Because Junio told me to: "We would prefer not to add inclusion of any system header files in random *.c files, as there often are system dependencies (order of inclusion, definition of feature macros, etc.) we would rather want to encapsulate in one place, that is git-compat-util.h."

Show 36 quoted lines

> > --- a/refs.c
> > +++ b/refs.c
> > @@ -24,6 +24,25 @@ static unsigned char refname_disposition[256] = {
> >  	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4
> >  };
> >  
> > +static int check_refname_component_trailer(const char *cp, const char *refname, int flags)
> > +{
> > +	if (cp == refname)
> > +		return 0; /* Component has zero length. */
> > +	if (refname[0] == '.') {
> > +		if (!(flags & REFNAME_DOT_COMPONENT))
> > +			return -1; /* Component starts with '.'. */
> > +		/*
> > +		 * Even if leading dots are allowed, don't allow "."
> > +		 * as a component (".." is prevented by a rule above).
> > +		 */
> > +		if (refname[1] == '\0')
> > +			return -1; /* Component equals ".". */
> > +	}
> > +	if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
> > +		return -1; /* Refname ends with ".lock". */
> > +	return cp - refname;
> > +}
> > +
> >  /*
> >   * Try to read one refname component from the front of refname.
> >   * Return the length of the component found, or -1 if the component is
> > @@ -37,7 +56,7 @@ static unsigned char refname_disposition[256] = {
> >   * - it ends with ".lock"
> >   * - it contains a "\" (backslash)
> >   */
> > -static int check_refname_component(const char *refname, int flags)
> > +static int check_refname_component_1(const char *refname, int flags)
> The name check_refname_component_1() doesn't tell too much,
> (check_refname_component_sse42()  or check_refname_component_nonsse42() say more)

I'll go with "_bytewise", since that's how it works.

> can I suggest to move all SSE code out to a file under compat/,
> like compat/refs_sse42.c, or something similar ?

Since this is a relatively small section of code, I think that would be overkill. Does anyone else have an opinion?