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

[Vladimir Kozlov]

On 7/12/17 11:33 AM, Erik Österlund wrote:
> Hi Doug,
> 
> Thanks for the review.
> 
> On 2017-07-12 20:04, Doug Simon wrote:
>> Hi Erik,
>>
>>> On 12 Jul 2017, at 17:49, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>>
>>> 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.
>> The current webrev looks good to me.
> 
> Glad to hear it!
> 
>>
>>>> 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?
>> JVMCI initialization is lazy (happens at first first top tier compilation request) so as to perturb VM startup as little as possible. As such, JVMCI compiler (e.g., Graal) checks against the VM 
>> configuration cannot occur during VM startup. Having Graal AOT compiled in the JDK may solve this but not every use of JVMCI should require AOT compilation of the compiler.
> 
> I agree. Yet JVM argument consistency checks can happen at the very start of bootstrapping. So what I propose is to perform simple consistency checks like (EnableJVMCI || UseAOT) -> 
> !UseJVMCIIncompatibleGC, if a new hypothetical GC called JVMCIIncompatibleGC was created. That way, the VM (create_vm) is simply not allowed to start if an incompatible combination of compiler and GC 
> is selected, so that create_vm does not return successfully only to later find out that "oops, actually the selected GC doesn't really work with the selected compiler, how about you try handling that 
> error at runtime after the server already started working". Hope I am making sense.

Yes, we can do that. There are already some checks dome in arguments.cpp related to EnableJVMCI.

Thanks,
Vladimir

> 
> Thanks,
> /Erik
> 
>> -Doug
>>
>>> 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
>