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

[Rahul Raghavan]

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);
   }
   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> 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
>