RFR (S): 8184269: JVMCI CompilerToVM::Data::initialize() should use BarrierSet fake RTTI to identify card table barrier sets

[Erik Österlund]

Hi Vladimir,

Thank you for the review.

On 2017-07-12 17:16, Vladimir Kozlov wrote:
> I think this code was made before 8069016: "Add BarrierSet downcast 
> support" but pushed later. As result it was not fixed by 8069016.
>
> Changes seems fine to me but Doug or someone from Labs may want to 
> look on it too.
> One thing - we may want to preserve JVMCI_ERROR() to check known by 
> Graal concrete barriers.

I did think about the JVMCI_ERROR() check. Here is a summary of my thoughts:
1) The current check is strange. It explicitly does not complain if the 
ModRef barrier set is selected. However, that is not a concrete barrier 
set and if it was indeed found to be the selected barrier set, it would 
most definitely be an error, and the JVM would arguably not work.
2) I do not think that the error checking for finding incompatible 
barrier sets should be done here in the first place. It should be done 
earlier.
Current collectors all support JVMCI. If a hypothetical new GC did not 
support JVMCI, e.g., does not provide enough information to JVMCI to 
make it possible for JVMCI compilers to JIT bytecode, and a user selects 
that hypothetical GC in combination with e.g. -XX:+UseJVMCICompiler, 
then that is already an invalid JVM argument combination, and it should 
not even be possible to create_vm with invalid JVM arguments.
Therefore, I think such sanity checking for JVMCI compliance should be 
done earlier in the bootstrapping during argument parsing, so that once 
bootstrapping is done, the invariant that we have a sane GC and compiler 
combination is already established. Then when this code is run, we 
already know that the user has selected a GC compatible with JVMCI, 
rather than sprinkling the code after create_vm with error checks, 
checking for conditions due to invalid JVM arguments that should never 
have been allowed.
3) The contract in hotspot is between the GC and JVMCI, not Graal. That 
is, the question we should ask ourselves in hotspot during this 
bootstrapping is whether a GC provides enough information to JVMCI to 
allow external JIT compilation, not whether Graal will do the right 
thing. Then it has to be the responsibility of external JITs like Graal 
to correctly detect what GCs it supports, rather than for the GCs to 
detect what external compilers it supports through JVMCI.

Due to the above reasoning, I believe it is best not to perform the 
sanity check here that a valid BarrierSet was provided. If an invalid GC 
and compiler combination exists, then that should be validated before 
create_vm succeeds during argument parsing, if such invalid combinations 
are introduced. And after bootstrapping, we should be able to trust that 
we do not at runtime need to detect incompatible BarrierSet and compiler 
combinations. Do you agree?

Thanks,
/Erik

> Thanks,
> Vladimir
>
> On 7/12/17 3:47 AM, Erik Österlund wrote:
>> Hi,
>>
>> Bug ID:
>> https://bugs.openjdk.java.net/browse/JDK-8184269
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8184269/webrev.00/
>>
>> The CompilerToVM::Data::initialize() member function of JVMCI 
>> identifies barrier sets with cards by taking the current BarrierSet 
>> and in a switch statement enumerating if it is in the set of all 
>> BarrierSets (except ModRef for some odd reason).
>> This switch statement includes both concrete barrier sets and 
>> abstract barrier sets that can never be selected.
>> This seems like the wrong way of doing things, and FakeRTTI should be 
>> used instead to see if the current barrier set is-a 
>> CardTableModRefBS, which is the condition for populating the card 
>> size and card table base address fields with sensible values.
>>
>> Thanks,
>> /Erik