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

[Doug Simon]

> On 13 Jul 2017, at 11:25, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> Hi Doug,
> 
> On 2017-07-12 23:14, Doug Simon wrote:
>>> On 12 Jul 2017, at 21:41, Erik Osterlund <erik.osterlund at oracle.com> wrote:
>>> 
>>> Hi Doug,
>>> 
>>>> On 12 Jul 2017, at 21:16, Doug Simon <doug.simon at oracle.com> wrote:
>>>> 
>>>> 
>>>>> On 12 Jul 2017, at 20:33, Erik Österlund <erik.osterlund at oracle.com> 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, that all makes sense and is a good idea.
>>>> 
>>>> I just wanted to make it clear that a lazily initialized JVMCI compiler can still bring the VM down after bootstrapping.
>>> Sorry, I am not sure I understand this. Perhaps there is a misunderstanding.
>>> Are you saying that despite making sure that JVMCI is disabled during bootstrapping,
>> What do you mean by disabled? I understand "That way, the VM (create_vm) is simply not allowed to start if..." to mean that the VM will exit.
> 
> I mean forcing EnableJVMCI to false if the selected GC does not support JVMCI.
> 
>> 
>>> the first top tier compilation (which happens later) may kick off the JVMCI compiler (despite being disabled) and bring down the VM? That sounds a bit weird to me. So it seems likely that I misunderstood you.
>> The confusion may stem from the two types of JVMCI initialization that takes place. The first is "native" initialization and the second is "java" initialization. The native initialization happens during create_vm and includes things like allocating the native JVMCICompiler object (jvmciCompiler.hpp). The consistency check you mention would happen during this phase or even earlier during argument validation. The "java" initialization phase is when the VM first calls into the Java part of JVMCI. The latter happens at the first call to JVMCICompiler::compile_method. Since Graal is only initialized during the "java" JVMCI initialization, that's the first chance it has to validate against VM configuration.
> 
> Right, gotacha. So what I am saying is that during what you refer to as native initialization, we first check our flag consistency (arguments.cpp) and make sure that it is impossible that both EnableJVMCI is set and UseJVMCIIncompatibleGC is set at the same time. So if UseJVMCIIncompatibleGC is set, then EnableJVMCI must not be set. Therefore, the subsequent CompileBroker::compilation_init() call in create_vm that initializes the compilation system will not create a JVMCICompiler, because that is guarded by the following conditions in compilerBroker.cpp:537:
> 
> #if INCLUDE_JVMCI
>  if (EnableJVMCI) {
>    // This is creating a JVMCICompiler singleton.
>    JVMCICompiler* jvmci = new JVMCICompiler();
> 
>    if (UseJVMCICompiler) {
>      _compilers[1] = jvmci;
>      ...
> 
> This code will not be run as it is already established that EnableJVMCI is false. As we can see, after create_vm has returned successfully, it is therefore impossible that we have a JVMCICompiler set as the top level compiler for the CompileBroker, and the JVMCIIncompatibleGC selected at the same time. Therefore, what you refer to as java initialization, triggered after the first call to JVMCICompiler::compile_method(), should never be triggered at all if EnableJVMCI is false. Because the JVMCICompiler was never created during the native initialization, and never selected as the top level compiler. Another compiler will be called instead.
> 
> So the point I was trying to make is that if the native initialization is done right with the right consistency checks, it should not be possible that any subsequent JVMCI initialization triggered by the java initialization finds out that JVMCI is being used with a JVMCI incompatible GC. Because the combination of EnableJVMCI and UseJVMCIIncompatibleGC is not allowed during native initialization.
> 
> What might happen though, is that a JVMCI compatible GC is selected, the JVMCICompiler is used, JVMCICompiler::compile_method kicks off an outdated version of Graal, but while reading the hotspot configuration realizes that despite the GC supporting JVMCI and providing the information necessary to JIT the code, an incompatible version of Graal is being used that does not support the selected GC. But that is a completely different story, and something we can not check for in hotspot. In hotspot code we only need to determine the consistency of the relationship between JVMCI and the GC, not between the GC and the external compiler using JVMCI.
> 
> Do we agree about this?

100% - thanks for bringing us to the same page with such a clear explanation!

-Doug

>> -Doug
>> 
>>>>>> -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