Re: Optimize Critical Section Functions

[Andreas Mohr]

Hi,

On Fri, Aug 20, 2004 at 04:50:36PM +0100, Robert Shearman wrote:
> Index: wine/dlls/ntdll/critsection.c
> ===================================================================
> RCS file: /home/wine/wine/dlls/ntdll/critsection.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 critsection.c
> --- wine/dlls/ntdll/critsection.c	17 Jun 2004 23:11:08 -0000	1.26
> +++ wine/dlls/ntdll/critsection.c	20 Aug 2004 15:46:18 -0000
> @@ -36,6 +36,14 @@
>  WINE_DEFAULT_DEBUG_CHANNEL(ntdll);
>  WINE_DECLARE_DEBUG_CHANNEL(relay);
>
> +#ifdef __GNUC__
> +# define likely(x)   __builtin_expect(!!(x), 1)
> +# define unlikely(x) __builtin_expect(!!(x), 0)
> +#else
> +# define likely(x)   !!(x)
> +# define unlikely(x) !!(x)
> +#endif
> +

Umm, this should go into a global header file, right?
The theory being that it would be useful at many more performance critical
Wine places...

But I have to admit that I don't know which header file this should be...

Andreas Mohr

Optimize Critical Section Functions

[Michael Stefaniuc]

Andreas Mohr wrote:
> Hi,
>
> On Fri, Aug 20, 2004 at 04:50:36PM +0100, Robert Shearman wrote:
> > Index: wine/dlls/ntdll/critsection.c
> > ===================================================================
> > RCS file: /home/wine/wine/dlls/ntdll/critsection.c,v
> > retrieving revision 1.26
> > diff -u -p -r1.26 critsection.c
> > --- wine/dlls/ntdll/critsection.c	17 Jun 2004 23:11:08 -0000	1.26
> > +++ wine/dlls/ntdll/critsection.c	20 Aug 2004 15:46:18 -0000
> > @@ -36,6 +36,14 @@
> > WINE_DEFAULT_DEBUG_CHANNEL(ntdll);
> > WINE_DECLARE_DEBUG_CHANNEL(relay);
> >
> > +#ifdef __GNUC__
> > +# define likely(x)   __builtin_expect(!!(x), 1)
> > +# define unlikely(x) __builtin_expect(!!(x), 0)
> > +#else
> > +# define likely(x)   !!(x)
> > +# define unlikely(x) !!(x)
> > +#endif
> > +
>
> Umm, this should go into a global header file, right?
> The theory being that it would be useful at many more performance critical
> Wine places...
>
> But I have to admit that I don't know which header file this should be...
> Like in the Linux kernel (where this seems to come from) in compiler.h 
> or something like that.

bye
	michael


-- 
Michael Stefaniuc               Tel.: [phone]
System Administration           Fax.: [phone]
Red Hat GmbH                    Email: mstefani(a)redhat.com
Hauptstaetterstr. 58            http://www.redhat.de/
D-70178 Stuttgart

Optimize Critical Section Functions

[Alexandre Julliard]

Michael Stefaniuc <mstefani(a)redhat.com> writes:
> Andreas Mohr wrote:
> > Umm, this should go into a global header file, right?
> > The theory being that it would be useful at many more performance critical
> > Wine places...
> > But I have to admit that I don't know which header file this should
> > be...
> > Like in the Linux kernel (where this seems to come from) in compiler.h
> > or something like that.

No, I don't think we want to do that, it would only encourage people
to sprinkle this stuff all over the place believing they are
"optimizing" the code. This should only be added in places where it
makes a significant difference, demonstrated with benchmark results.

-- 
Alexandre Julliard
julliard(a)winehq.org

Optimize Critical Section Functions

[Robert Shearman]

Alexandre Julliard wrote:
> Michael Stefaniuc <mstefani(a)redhat.com> writes:
> > Andreas Mohr wrote:
> > > Umm, this should go into a global header file, right?
> > > The theory being that it would be useful at many more performance critical
> > > Wine places...
> > > But I have to admit that I don't know which header file this should
> > > be...
> >
> > Like in the Linux kernel (where this seems to come from) in compiler.h
> > or something like that.
>
> No, I don't think we want to do that, it would only encourage people
> to sprinkle this stuff all over the place believing they are
> "optimizing" the code. This should only be added in places where it
> makes a significant difference, demonstrated with benchmark results.

Yes, I agree. In fact the patch actually decreases performance.
Test: 10,000,000 calls to EnterCriticalSection & LeaveCriticalSection, 
simulating fast path on critical sections.
Results:
Win2k - 1000ms
Wine w/ patch - 1346ms
Wine w/o patch - 1294ms

I think the best way to improve performance of the critical section 
functions is to rewrite them in assembly and I will submit a patch doing 
so when I have a spare moment.

Rob

Optimize Critical Section Functions

[Alexandre Julliard]

Robert Shearman <rob(a)codeweavers.com> writes:
> Yes, I agree. In fact the patch actually decreases performance.
> Test: 10,000,000 calls to EnterCriticalSection & LeaveCriticalSection,
> simulating fast path on critical sections.
> Results:
> Win2k - 1000ms
> Wine w/ patch - 1346ms
> Wine w/o patch - 1294ms
>
> I think the best way to improve performance of the critical section
> functions is to rewrite them in assembly and I will submit a patch
> doing so when I have a spare moment.

I don't think that's necessary either, unless you have good evidence
that the critical section non-contention path represents a significant
amount of the total time spent by an application. IMO we have much
bigger performance issues to solve first, before we make the critical
section code unreadable just to save a couple of cycles.

-- 
Alexandre Julliard
julliard(a)winehq.org

Optimize Critical Section Functions

[Mike Hearn]

> Win2k - 1000ms
> Wine w/ patch - 1346ms
> Wine w/o patch - 1294ms

That's a fascinatingly round number for Win2K, isn't it?

I suspect the slowness is caused by the involution: any cycles saved 
from the branch hinting is lost in the double-not.

thanks -mike