Re: [PATCH] Optimise memset on i386

[Colin Watson]

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  <address@hidden>
2010-07-23  Colin Watson  <address@hidden>

        * kern/misc.c (grub_memset): Optimise to reduce cache stalls.

=== 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;
 }

-- 
Colin Watson                                       address@hidden