[14]RFR: 8227439: Turn off AOT by default

[Rahul Raghavan]

Thanks for review.

Pushed latest webrev -
http://cr.openjdk.java.net/~rraghavan/8227439/webrev.02/

-Rahul

On 27/07/19 8:00 AM, Igor Ignatyev wrote:
> 
> 
>> On Jul 26, 2019, at 6:56 PM, Vladimir Kozlov 
>> <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>> wrote:
>>
>> On 7/25/19 12:11 AM, Rahul Raghavan wrote:
>>> Hi,
>>> Thanks Igor, Vladimir for the review comments.
>>> Please review following updates.
>>> >> <webrev> - http://cr.openjdk.java.net/~rraghavan/8227439/webrev.01/
>>> > You don't need to add -J-XX:+UnlockExperimentalVMOptions to JAOTC_OPTS
>>> > in aot/scripts/ scripts because you added it to jaotc launcher
>>> > (Launcher-jdk.aot.gmk).
>>> >
>>> Okay, will removed -J-XX:+UnlockExperimentalVMOptions added to 
>>> JAOTC_OPTS in -
>>> aot/scripts/ - build-bootmodules.sh, build-jdk.vm-modules.sh and 
>>> test-jaotc.sh
>>> > As UseAOT used to be true by default,
>>> > you should add to all places where AOTLibrary is used,
>>> > e.g. in make/RunTests.gmk,
>>> > and as it seems like an easy error (for our users) to make,
>>> > I think we need to check that AOTLibrary has value
>>> > only if UseAOT is true
>>> > and generate an error at initialization time if it's not a case.
>>> >
>>> Understood the review points by Igor.
>>> Found following current implementation of AOTLoader::initialize()
>>> http://hg.openjdk.java.net/jdk/jdk/file/9b6d4e64778c/src/hotspot/share/aot/aotLoader.cpp#l110
>>> void AOTLoader::initialize() {
>>> ......
>>> if (FLAG_IS_DEFAULT(UseAOT) && AOTLibrary != NULL) {
>>> // Don't need to set UseAOT on command line when AOTLibrary is specified
>>> FLAG_SET_DEFAULT(UseAOT, true);
>>
>> Please keep current implementation. No need to change it.
>> Examples in AOT JEP use AOTLibrary flag without UseAOT. Lets keep it 
>> this way.
> 
> Agree, I somehow overlooked this piece in AOTLoader::initialize.
> 
> -- Igor
>>
>> Vladimir
>>
>>> }
>>> if (UseAOT) {
>>> .......
>>> warning("EagerInitialization is not compatible with AOT (switching 
>>> AOT off)");
>>> .......
>>> warning("JVMTI capability to post breakpoint is not compatible with 
>>> AOT (switching AOT off)");
>>> .......
>>> warning("-Xint is not compatible with AOT (switching AOT off)");
>>> // Scan the AOTLibrary option.
>>> if (AOTLibrary != NULL) {
>>> .......
>>> }
>>> // Load well-know AOT libraries from Java installation directory.
>>> .......
>>> }
>>> }
>>> }
>>> So current design with my webrev.01 is to make UseAOT automatically 
>>> true if some AOTLibrary is specified and no explicit -UseAOT.
>>> Should we change this to -
>>> [src/hotspot/share/aot/aotLoader.cpp]
>>> if (FLAG_IS_DEFAULT(UseAOT) && AOTLibrary != NULL) {
>>> -    // Don't need to set UseAOT on command line when AOTLibrary is 
>>> specified
>>> -    FLAG_SET_DEFAULT(UseAOT, true);
>>> +    if (UseAOT == tue) {
>>> +      // Don't need to set UseAOT on command line when AOTLibrary is 
>>> specified
>>> +      FLAG_SET_DEFAULT(UseAOT, true);
>>> +    }
>>> +    else {
>>> +      warning("AOTLibrary specified without explicitly switching on 
>>> UseAOT (ignoring AOTLibrary)");
>>> +    }
>>> OR instead of new warning above, throw error -
>>> +    else {
>>> +      fatal("AOTLibrary specified without explicitly switching on 
>>> UseAOT");
>>> +      vm_exit(1);
>>> +    }
>>> And then also make sure +UseAOT is added to all places where 
>>> AOTLibrary is used.
>>> Thanks,
>>> Rahul
>>> On 19/07/19 10:02 PM, Igor Ignatyev wrote:
>>>> Hi Rahul,
>>>>
>>>> thanks for taking care of this and all the tests, appreciate that. 
>>>> As UseAOT used to be true by default, you should add to all places 
>>>> where AOTLibrary is used, e.g. in make/RunTests.gmk, and as it seems 
>>>> like an easy error (for our users) to make, I think we need to check 
>>>> that AOTLibrary has value only if UseAOT is true and generate an 
>>>> error at initialization time if it's not a case. this will help us 
>>>> and all the users to spot places where AOT was expected to kick in.
>>>>
>>>> (I have not looked at all the changed files yet)
>>>>
>>>> Thanks,
>>>> -- Igor
>>>>
>>>>> On Jul 19, 2019, at 12:52 AM, Rahul Raghavan 
>>>>> <rahul.v.raghavan at oracle.com <mailto:rahul.v.raghavan at oracle.com>> 
>>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Please review the following fix changeset and
>>>>> related release-note task (8228418).
>>>>>
>>>>>
>>>>> <webrev> - http://cr.openjdk.java.net/~rraghavan/8227439/webrev.01/
>>>>>
>>>>>
>>>>> # https://bugs.openjdk.java.net/browse/JDK-8227439
>>>>> (Turn off AOT by default)
>>>>>
>>>>> # CSR - https://bugs.openjdk.java.net/browse/JDK-8227833
>>>>>
>>>>> # RN - https://bugs.openjdk.java.net/browse/JDK-8228418
>>>>>
>>>>>
>>>>> -- AOT support related flags `UseAOT`, `PrintAOT` and `AOTLibrary` 
>>>>> are made experimental;
>>>>> and `UseAOT` flag is turned off by default.
>>>>> Also added required -XX:+UnlockExperimentalVMOptions, related 
>>>>> changes in tests.
>>>>>
>>>>> -- Got approval for related CSR - 8227833
>>>>> and created Release-Note task as commented - 8228418.
>>>>>
>>>>> -- tried tests --job hs-tier4,hs-tier4-graal,hs-tier6,hs-tier6-graal.
>>>>> Could not find any issues due to proposed changes.
>>>>>
>>>>>
>>>>> Please let me know if missed any changes or testing.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Rahul
>