RFR: 8069016: Add BarrierSet downcast support

[Jon Masamitsu]

On 2/20/2015 11:48 AM, Kim Barrett wrote:
> Jon, thanks for looking at this.
>
> On Feb 20, 2015, at 3:04 AM, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
>>
>> http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00/src/share/vm/utilities/fakeRttiSupport.hpp.html
>>
>>   86   TagType _ctag;
>>
>> I'd prefer that the variable be named _concrete_tag
> Sure, I'll change to _concrete_tag.
>
>> http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00/src/share/vm/gc_implementation/parallelScavenge/cardTableExtension.hpp.udiff.html
>>
>> +      // Concrete tag should be BarrierSet::CardTableExtension.
>> +      // That will presently break things in a bunch of places though.
>>
>> Can you expand on these comments.  Where would the breakage occur?
>>
>>
>> http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00/src/share/vm/gc_implementation/parallelScavenge/psMarkSweep.cpp.udiff.html
>>
>> -    BarrierSet* bs = heap->barrier_set();
>> -    if (bs->is_a(BarrierSet::ModRef)) {
>> -      ModRefBarrierSet* modBS = (ModRefBarrierSet*)bs;
>> +    ModRefBarrierSet* modBS = barrier_set_cast<ModRefBarrierSet>(heap->barrier_set());
>>
>>
>> Is this one of the place that breaks because the barrier set is really a CardTableExtension?
>>
>> http://cr.openjdk.java.net/~kbarrett/8069016/webrev.00/src/share/vm/memory/cardTableModRefBS.hpp.udiff.html
>>
>> +      // Concrete tag should be BarrierSet::CardTableForRS.
>> +      // That will presently break things in a bunch of places though.
>>
>>
>> Expand on this comment also about what will break.
> The concrete tag is used as a dispatch key in many places.
>
> I'm not sure all of those places handle BarrierSet::CardTableExtension
> properly.  One place that I think would probably "only" be a
> performance regression is barrierSet.inline.hpp, where the concrete
> tag is used to decide whether to devirtualize the call.
>
> Since BarrierSet::CardTableForRS is new and not propogated everywhere,
> using it as the concrete tag would certainly break things badly for
> relevant collectors.
>
> There is a CR for the CardTableExtension case:
> https://bugs.openjdk.java.net/browse/JDK-8072817
>
> My current thinking is to address some of this by modifications to the
> barrier set class hierarchy and the elimination of the use of the
> concrete tag in favor of (now fast) is_a() tests.  For example, change
> the devirtualization in barrierSet.inline.hpp to use
> is_a(BarrierSet::CardTableModRef).  That doesn't presently work,
> because the G1 barrier sets are derived from there; changing that
> would be one of the class hierarchy changes I'm thinking about.
>
> A goal of introducing barrier_set_cast is to make such refactorings
> easier.
>
Thanks for the additional information.  I would be happy with a comment
along the lines of

  // Concrete tag should be BarrierSet::CardTableExtension.
  // The concrete tag is used as a dispatch key in many places
// (some yet TBD) and CardTableExtension does not correctly
// dispatch in some of those uses.  This will be addressed is
// as part of a reorganization of the BarrierSet hierarchy.

You can choose the exact words.   I just thought "breaks
in a bunch of places" a little lean on information.

Reviewed.

Jon