Re: [PATCH] Optimise memset on i386

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2655 bytes --]

On 07/23/2010 02:14 PM, Colin Watson wrote:
> On Fri, Jun 25, 2010 at 08:27:20PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>   
>>> +void *
>>> +grub_memset (void *s, int c, grub_size_t n)
>>> +{
>>> +  unsigned char *p = (unsigned char *) s;
>>> +
>>> +  while (n--)
>>> +    *p++ = (unsigned char) c;
>>> +
>>> +  return s;
>>> +}
>>>       
>> Attached is a possible generic implementation. Not even compile-tested.
>> Could you test it and compare disasm of this version on i386 and glibc
>> i386 version. Perhaps they are equivalent. If they are for maintainance
>> reasons it's better to always use generic one.
>>     
> Thanks for this, and sorry for my delay.
>
> I haven't compared the disassembly, but once I fixed up your code it
> performed within about 10% of the optimised version I posted before, so
> about 640ms original, 160ms with i386-specific memset, and 176ms with
> your generic memset.  Even though that isn't quite equivalent, I don't
> think that 16ms is going to be a major problem, and so I would suggest
> going with the generic version.  Please check the following.
>
> 2010-07-23  Vladimir Serbinenko  <phcoder@gmail.com>
> 2010-07-23  Colin Watson  <cjwatson@ubuntu.com>
>
> 	* kern/misc.c (grub_memset): Optimise to reduce cache stalls.
>
>   
Go ahead.
> === modified file 'kern/misc.c'
> --- kern/misc.c	2010-07-02 17:35:07 +0000
> +++ kern/misc.c	2010-07-23 11:05:32 +0000
> @@ -518,12 +518,39 @@ grub_strndup (const char *s, grub_size_t
>  }
>  
>  void *
> -grub_memset (void *s, int c, grub_size_t n)
> +grub_memset (void *s, int c, grub_size_t len)
>  {
> -  unsigned char *p = (unsigned char *) s;
> +  void *p = s;
> +  grub_uint8_t pattern8 = c;
>  
> -  while (n--)
> -    *p++ = (unsigned char) c;
> +  if (len >= 3 * sizeof (unsigned long))
> +    {
> +      unsigned long patternl = 0;
> +      grub_size_t i;
> +
> +      for (i = 0; i < sizeof (unsigned long); i++)
> +	patternl |= ((unsigned long) pattern8) << (8 * i);
> +
> +      while (len > 0 && (((grub_addr_t) p) & (sizeof (unsigned long) - 1)))
> +	{
> +	  *(grub_uint8_t *) p = pattern8;
> +	  p = (grub_uint8_t *) p + 1;
> +	  len--;
> +	}
> +      while (len >= sizeof (unsigned long))
> +	{
> +	  *(unsigned long *) p = patternl;
> +	  p = (unsigned long *) p + 1;
> +	  len -= sizeof (unsigned long);
> +	}
> +    }
> +
> +  while (len > 0)
> +    {
> +      *(grub_uint8_t *) p = pattern8;
> +      p = (grub_uint8_t *) p + 1;
> +      len--;
> +    }
>  
>    return s;
>  }
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]