[llvm-commits] [PATCH] Improved threading support on Windows

[Aaron Ballman]

On Tue, Aug 23, 2011 at 5:34 AM, NAKAMURA Takumi <geek4civic at gmail.com> wrote:
> Good evening, Aaron!
>
> About lib/Support/Windows/RWMutex.inc;
>
>  - Would you like to try describing rwmutex on windows xp?
>    Oh yeah, I don't have any Windows XP hosts any more. :(

In code or on the mailing list?

Windows XP doesn't have slim reader/writer locks (they're a Vista and
up API).  So the RWMutex object supports reader/writer locks when
possible, but fallsback on the more heavy-handed original
implementation, which uses vanilla critical section objects.  This
implementation is correct on XP, just not as efficient as on Vista and
higher.  On Vista and up, you can have multiple non-blocked readers at
once, but on XP you will only be able to have one reader at a time.

If you'd like me to update the comments in the code, I certainly can.

>  - I think, rather to refer to kernel32.dll, GetModuleHandle(NULL)
> would be more enough.
>    How do you think?

I've never been too happy with that convention.  While it would
certainly work because Kernel32 is mapped into every executable's
process space, I don't think the code would be as clear where the
functions are coming from.  Also, there's no performance penalty for
calling LoadLibrary vs GetModuleHandle( NULL ) that I've ever heard
of.  Since the library is already loaded into memory, the loader will
traverse the list of loaded modules in the PEB, see Kernel32 (early
on) and return the module handle from there.

>  - It would be happier for us to have generic "delayed dll resolver"
> for NT5.1-unavailable entries.
>    I think it might be the global ctor. How do you think?

That's not a bad idea.  I did a quick search of the code base, and
this is the first case we're using lazy loading for OS APIs.  My
personal feeling is: if we need to do this a second time, a helper API
could be designed to make this cleaner.  But since this is only
happening once, the helper may be overkill.

>  - How about to split RWMutexImpl to Windows XP and higher?

I originally looked into that, but it would be a larger change than I
was comfortable with.  Right now, the Impl is a concrete class, and an
OS version split would require it to be a bridge.  If you think it's a
better approach though, I can certainly tackle it.

>  - Could you consider unittests for rwmutex?

Yes, I could probably write some up, at least for testing them on
Windows.  Can you recommend where I should put the unit tests though
(I've not yet done something like this for LLVM).

Thanks for the review!

~Aaron