RFR: 8163897: oop_store has unnecessary memory barriers

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8163897: oop_store has unnecessary memory barriers

Kim Barrett
Please review this change to the oop_store function template, which
removes some unnecessary memory barriers, moves CMS-specific code into
GC-specific (though not completely CMS-specific) areas, and cleans up
the API a bit.  See the CR for more details about the problems.

[Note: CTMRBS expands to CardTableModRefBS below.]

As a preliminary cleanup, CTMRBS::inline_write_ref_field has been
merged into it's only caller, CTMRBS::write_ref_field_work.  This left
the file gc/shared/cardTableModRefBS.inline.hpp effectively empty, so
it has been removed.  As a related cleanup,
CTMRBS::inline_write_ref_field_pre was found to be unused and has been
removed.

The volatile overload for oop_store has been renamed to
release_oop_store, to correspond to its purpose.  oop_store no longer
examines always_do_update_barrier to conditionally call the (now
renamed) volatile overload.  The only other caller of the volatile
overload was release_obj_field_put, which has been updated for the new
name.

The release argument for BarrierSet::write_ref_field and all the
related implementation has been removed.  Instead,
CTMRBS::write_ref_field_work now uses a release_store to mark the card
if card marking is required to be ordered after the value store,
e.g. for CMS, per the value of always_do_update_barrier.

Finally, the global variable always_do_update_barrier, which was only
needed for CMS, has been replaced with member variable
CTMRBS::_requires_ordered_marking (with accessor functions).  (G1 had
commented out manipulation of this variable, added in commented out
state as part of fix for 6904516; looks like debugging leftovers.
Those have been removed.)

So we now have [release_]oop_store, which

(1) calls the barrier set's pre-barrier handler (which is a nop except for G1),

(2) then performs a [release_]store of the new value,

(3) and finally calls the barrier set's post-barrier handler.  The
post-barrier handler shared by Serial, Parallel, and CMS performs the
card marking with a release barrier when requested (only for CMS).

With these changes, a release store of the new value is only done when
that's what is actually required by the caller, without regard to some
hidden global variable.  Also with these changes, only CMS (not Serial
or Parallel) uses a release store for the card marking, and then only
when actually needed, irrespective of whether the value store needed
to be a release store.

Finally, _requires_ordered_marking is now only set true when both
UseConcMarkSweepGC and CMSPrecleaningEnabled are true, which matches
the behavior of JITed code.  Precleaning is what requires the
ordering, so there's no point if it's disabled.

CR:
https://bugs.openjdk.java.net/browse/JDK-8163897

Webrev:
http://cr.openjdk.java.net/~kbarrett/8163897/open.00/

Testing:
hs-tier1 through hs-tier5.


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8163897: oop_store has unnecessary memory barriers

Kim Barrett
> On Oct 23, 2017, at 1:44 AM, Kim Barrett <[hidden email]> wrote:
>
> Please review this change to the oop_store function template, which
> removes some unnecessary memory barriers, moves CMS-specific code into
> GC-specific (though not completely CMS-specific) areas, and cleans up
> the API a bit.  See the CR for more details about the problems.

Due to some miscommunication, Erik O and I have both developed
solutions to this.  Mine is a stand-alone piece of work for me, while
his is some number of changes in a long patch train.  In the interest
of not imposing possibly messy merging on Erik, I'm withdrawing this
RFR and reassigning the bug to him.