[aarch64-port-dev ] RFR(S): 8248657: Windows: strengthening in ThreadCritical regarding memory model

Thomas Stüfe thomas.stuefe at gmail.com
Sun Jul 12 05:26:44 UTC 2020


I agree with Kim, this is so much better.

I also like that lock_count now starts at 0.

And that we assert if initial event creation fails instead of just ignoring
it (Okay we would have noticed in ~ThreadCritical()).

Only small style nit (unrelated to this patch) is that IMHO we should use
TRUE or FALSE, not true or false, when facing win32 APIs taking BOOL.

This is okay from my side.

Cheers, Thomas




On Sat, Jul 11, 2020 at 4:17 PM Kim Barrett <kim.barrett at oracle.com> wrote:

> > On Jul 11, 2020, at 9:14 AM, David Holmes <david.holmes at oracle.com>
> wrote:
> >
> > Hi Ludovic,
> >
> > Sorry but this fix seems specific to the Windows-Aarch64 port work and
> as such should be fixed as part of that port when the JEP is approved and
> targeted.
> >
> > David
>
> I'm inclined to disagree with that assessment.
>
> This change seems to me to make the code noticeably simpler and easier
> to understand, while also making it platform-independent (eliminating
> a non-TSO race). Those seem like good things, regardless of any
> aarch64 port that might be coming.
>
> My only (very small) quibble with it is that I think it could be made
> simpler still by using a thread-safe function scoped static variable
> to control the initialization, rather than using InitOnceExecuteOnce.
> But unfortunately, that thread-safety guarantee is a C++11 feature. I
> expect that feature is present in VS 2015 or later, but until JEP 347
> gets integrated (very soon, I hope), we still support older versions.
>
> So I think this change looks good.
>
>
> > On 10/07/2020 8:19 am, Ludovic Henry wrote:
> >> Hello,
> >> This small fix is in the context of the larger support for
> Windows-AArch64. I am using Bernhard Urban's CR as I am currently not an
> author.
> >> ThreadCritical is used to synchronize the allocation of new Arena
> chunks. However, on platforms with weaker memory models than x86 (primarily
> ARM), the original ThreadCritical initialization code would be racy,
> leading to crashes. To fix that, we switch to initializing the
> ThreadCritical static data by using a functionally-sound Win32 API focused
> on initialization [1]. This approach also has the advantage of simplifying
> the code, and get it closer to how it is done on Linux.
> >> JBS: https://bugs.openjdk.java.net/browse/JDK-8248657
> >> Webrev: http://cr.openjdk.java.net/~burban/luhenry/8248657/webrev.00/
> >> Testing: jtreg:test/hotspot/jtreg:tier1, jtreg:test/jdk:tier1,
> jtreg:test/jdk:tier2, jtreg:test/langtools on Windows-x86_64, no regressions
> >> Thank you,
> >> --
> >> Ludovic
> >> [1]
> https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-initonceinitialize
>
>
>


More information about the hotspot-runtime-dev mailing list