RFR(S): 8153267: nmethod's exception cache not multi-thread safe

[Doerr, Martin]

Hi,

I think we have come to a common understanding and there was no complaint about my latest webrev:
http://cr.openjdk.java.net/~mdoerr/8153267_exception_cache/webrev.02/

Can I consider it reviewed?
Can somebody sponsor, please?

Thanks and best regards,
Martin


-----Original Message-----
From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Doerr, Martin
Sent: Donnerstag, 7. April 2016 12:52
To: Andrew Haley <aph at redhat.com>; Jamsheed C m <jamsheed.c.m at oracle.com>; hotspot-compiler-dev at openjdk.java.net
Subject: RE: RFR(S): 8153267: nmethod's exception cache not multi-thread safe

Hi Andrew, Jamsheed and all,

thank you very much for your input.

As Andrew, Jamsheed and I think, it's better to have a releasing store in increment_count().
Therefore, I have replaced the storestore barrier introduced with JDK-8143897  (even though this barrier was also correct).

My change still contains a releasing store for newly created ExceptionCache instances.
As Jamsheed has pointed out, this should not be strictly required as we have the other barrier. It may only produce additional false negatives on weak memory model platforms.
I think having the release doesn't hurt too much and makes the design a little cleaner.

I also added comments based on your input.

The new webrev is here:
http://cr.openjdk.java.net/~mdoerr/8153267_exception_cache/webrev.02/

Please review. I will also need a sponsor from Oracle, please.

Thanks again and best regards,
Martin


-----Original Message-----
From: Andrew Haley [mailto:aph at redhat.com] 
Sent: Donnerstag, 7. April 2016 12:14
To: Doerr, Martin <martin.doerr at sap.com>; Jamsheed C m <jamsheed.c.m at oracle.com>; hotspot-compiler-dev at openjdk.java.net
Subject: Re: RFR(S): 8153267: nmethod's exception cache not multi-thread safe

On 07/04/16 10:08, Doerr, Martin wrote:

> atomic update for the _count would only be required if there were
> multiply threads which attempt to increment it
> concurrently. However, updates are under lock, so we only have
> concurrent readers which is ok. 
> 
> I still think "volatile" does what we need here. Especially the xlC
> compiler on AIX tends to reload variables from memory. Exactly this
> can be prevented by making the field volatile.

I think your latest patch is OK.  Whether volatile is really good
enough, I don't know.  The new(ish) C++ memory model treats this as a
race, and therefore undefined behaviour.  Old C++ didn't have a memory
model, so the best we can do with racy code is guess about what our
compilers might do.

I certainly much prefer a release_store to the storestore fence used
in the fix for 8143897.

Andrew.