RFR(L) 8153224 Monitor deflation prolong safepoints (CR8/v2.08/11-for-jdk14)
David Holmes
david.holmes at oracle.com
Tue Nov 12 23:12:14 UTC 2019
Hi Dan,
On 13/11/2019 8:24 am, Daniel D. Daugherty wrote:
> Greetings,
>
> I'm only going to jump in on a single item here (at this time).
>
>
> On 11/11/19 9:03 AM, David Holmes wrote:
>> Hi Robbin,
>>
>> On 11/11/2019 10:41 pm, Robbin Ehn wrote:
>>>
>>> Also in this patch there is already Atomic::store/load on "volatile
>>> markWord _header;".
>>
>> And I've flagged the inappropriateness of using these with Dan. Though
>> I see we already have a couple of pre-existing occurrences which have
>> snuck in - again this seems to be a misunderstanding about the need
>> for Atomic use in these cases.
>
> I read this comment from Robbin and David's reply and my brain said: What?
> It might have been just the common three letter acronym, but I
> digress... :-)
First let me clarify that my comment quoted above may have been too
broad/general. I wasn't recalling specific changes where you added
Atomic::load/store but email exchanges where you said words to the
effect of "I could replace ... with Atomic::store ..." and I replied
that there was no need to use Atomic::load/store.
> So I searched the patch:
Thanks for digging that out, as I hadn't recalled those details.
Short version: I've agreed with Robbin that we should move to use
Atomic::load/store to get compiler-based-atomicity rather than relying
on use of "volatile" on variables. But for the purposes of this patch
(where Robbin made a number of suggestions on where to use
Atomic::load/store) that we try to limit using this new style to
inherently new code (ie lock-free list management) rather than
retrofitting all existing usages.
Hope that clarifies somewhat.
Thanks,
David
-----
> $ grep _header 11-for-jdk14.v2.08.full/open.patch | egrep 'store|load'
> assert(Atomic::load(&_header).value() != 0, "must be non-zero");
> + Atomic::store(markWord::zero(), &_header);
> - Atomic::store(markWord::zero(), &_header);
>
> Oh... that code... If you look at the 8153224 webrev you'll see that the
> ObjectMonitor::clear() function was refactored into two parts:
>
> ObjectMonitor::clear()
> ObjectMonitor::clear_using_JT()
>
> This line:
>
> Atomic::store(markWord::zero(), &_header);
>
> is in the original ObjectMonitor::clear() function and it is in
> the new ObjectMonitor::clear() function, but there's a lot of
> code motion in between those two functions so... diff took the
> easy way out and showed this:
>
> + Atomic::store(markWord::zero(), &_header);
>
> as a new line in the shorter, new clear() function
> and as a deleted line in the longer, old clear() function:
>
> - Atomic::store(markWord::zero(), &_header);
>
>
> Okay, so where did that come from? I've been tweaking a lot of
> ObjectMonitor code lately, but I don't think that line is mine...
>
> $ hg annot src/hotspot/share/runtime/objectMonitor.inline.hpp | grep
> 'Atomic::store(markWord::zero(), &_header);'
> 56006: Atomic::store(markWord::zero(), &_header);
>
> $ hg log -r 56006
> changeset: 56006:90ead0febf56
> user: stefank
> date: Tue Aug 06 10:48:21 2019 +0200
> summary: 8229258: Rework markOop and markOopDesc into a simpler mark
> word value carrier
>
> Okay, I remember this bug:
>
> https://bugs.openjdk.java.net/browse/JDK-8229258
>
> and I even reviewed it... :-) It looks like the reviewers are:
>
> > Reviewed-by: rkennke, coleenp, kbarrett, dcubed
>
>
> Roman posted this comment during the 8229258 review:
>
> On 8/15/19 1:06 PM, Roman Kennke wrote:
>
>> Out of curiosity, what's with the changes in objectMonitor.inline.hpp to
>> access the markWord atomically?:
>>
>> -inline markOop ObjectMonitor::header() const {
>> - return _header;
>> +inline markWord ObjectMonitor::header() const {
>> + return Atomic::load(&_header);
>> }
>>
>> I guess this is good (equal or stronger than before) but is there a
>> rationale behind these changes?
>
> and Stefan K replied with this:
>
> On 8/15/19 3:26 PM, Stefan Karlsson wrote:
>
>> Ahh. Right. That was done to solve the problems I were having with
>> volatiles. For example:
>> src/hotspot/share/runtime/objectMonitor.inline.hpp:38:10: error:
>> binding reference of type 'const markWord&' to 'const volatile
>> markWord' discards qualifiers
>> return _header;
>>
>> and:
>> src/hotspot/share/runtime/basicLock.hpp:40:74: error: implicit
>> dereference will not access object of type ‘volatile markWord’ in
>> statement [-Werror]
>> void set_displaced_header(markWord header) {
>> _displaced_header = header; }
>>
>> Kim suggested that the fact that these fields were volatile was an
>> indication that we should be doing some kind of atomic/ordered
>> operation. By replacing these loads and stores with calls to the
>> Atomic APIs, and providing the
>> PrimitiveConversions::Translate<markWord> specialization, we could
>> solve that problem.
>
> So it appears that Stefan has a good rationale for making the
> Atomic::load() and Atomic::store() changes with the _header field.
> Since I've added more volatile fields to ObjectMonitor, it would
> follow that I should make similar changes...
>
> However, it's not clear that David agrees with the above change so
> I'm hesitant to make the similar changes to my patch...
>
> How do we resolve this issue?
>
> Dan
More information about the hotspot-runtime-dev
mailing list