RFR (S): 8189355: Cleanup of BarrierSet barrier functions

[Erik Österlund]

Hi Kim,

On 2017-10-18 03:29, Kim Barrett wrote:
>> On Oct 17, 2017, at 6:27 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>
>> Hi,
>>
>> The BarrierSet class has a bunch of unused or used in a seemingly nonsense way code. Some methods are used but only used in assertion, and are expected to return true (the has_*_opt methods). Others describe barriers that are never called from anywhere. This might have made sense at some point, but arguably does not any more. There is also an enum called Flags not referenced from anywhere.
>>
>> I would also like to get rid of devirtualize_reference_writes() which devirtualizes calls to the post-write barrier for card marking specifically by loading and comparing the type of barrier set. I have run a bunch of benchmarks and this optimization did not seem to provide any benefit. It will also soon be superseded by the new GC barrier interface.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8189355
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8189355/webrev.00/
>>
>> Thanks,
>> /Erik
> ------------------------------------------------------------------------------
> src/hotspot/cpu/arm/stubGenerator_arm.cpp
>
> gen_write_ref_array_pre_barrier is now unconditionally generating a
> call to static_write_ref_array_pre, where before the generation of
> that call was conditional on has_write_ref_pre_barrier.  This change
> doesn't seem right.
>
> All other cpu's seem to make this decision via dispatch on gs->kind(),
> with G1SATBCTLogging generating a call to static_write_ref_array_pre,
> and all others generating no code.

Fixed. While the unconditional call is harmless (and will be reworked 
soon), I made it conditional again in the latest webrev (in the reply to 
Aleksey who noticed this first).

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/barrierSet.hpp
>
> What happened to _max_covered_regions?
> Ah, moved to CardTableModRefBS.hpp

Yes.

> See related bugs and email thread for this bug:
> https://bugs.openjdk.java.net/browse/JDK-8064721
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2014-November/011279.html
> Or don't, if you think you'll feel depressed by discussion of (still)
> leftover remnents of permgen.

I accidentally clicked the links, then closed those windows before 
reading too much.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/cardTableModRefBS.hpp
>
> I was going to ask why change the indentation of the access qualifiers
> in CardTableModRefBS, but then noticed it was using a mix of 0 and 1
> space indent.

Exactly.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/modRefBarrierSet.hpp
>
> This class looks pretty nearly useless now.  And the comment
> describing it seems questionable, since it mentions use of a card
> table, and there's nothing like that at this layer.
>
> Maybe this class should just go away?  That can be done in a followup
> cleanup.

I am still not sure if we want to keep this class or not, so I do not 
want to remove it just yet. Perhaps we do that down the road.

Thanks for the review.

/Erik