Thank you, David On 11/28/17 8:57 PM, David Holmes wrote:

Hi Vladimir,

On 29/11/2017 9:51 AM, Vladimir Kozlov wrote:

...

http://cr.openjdk.java.net/~kvn/8191788/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8191788

This is redo of JDK-8190975 [1] fix which added jdk.internal.vm.compiler to tests which have --limit-modules option. Unfortunately tests start failing on SPARC where Graal (jdk.internal.vm.compiler) is not available. JDK-8191653 [2] reversed 8190975 changes to fix that problem.

This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM only when Graal is used: when it is explicitly specified with -Djvmci.Compiler=graal or in default case and when UseJVMCICompiler is true.

This seems a reasonable way to handle this - at least for now.

Typo:

3379     // and --limit-modules are spcified.

spcified -> specified

Will fix. Vladimir

Thanks, David -----

...

I tested the fix with failed tests from JDK-8190975 which are mostly AppCDS tests in test/runtime/appcds/jigsaw and also test/jdk/java/lang/String/concat/WithSecurityManager.java

I think this fix may break upgradeable status of Graal (for Oracle Labs version of Graal). But it should be fine since it is only used with --limit-modules which is not used by Labs.

Thanks, Vladimir

[1] http://hg.openjdk.java.net/jdk/hs/rev/8deb7919d118 [2] http://hg.openjdk.java.net/jdk/hs/rev/b38d8aadcada

Thank you, Calvin On 11/28/17 10:15 PM, Calvin Cheung wrote:

Hi Vladimir,

Fix looks good to me.

Could you break up lines #3386 (104 chars) and #3391 (146 chars)?

I will do that. Vladimir

No need to send another webrev for the above change.

thanks, Calvin

On 11/28/17, 3:51 PM, Vladimir Kozlov wrote:

...

http://cr.openjdk.java.net/~kvn/8191788/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8191788

This is redo of JDK-8190975 [1] fix which added jdk.internal.vm.compiler to tests which have --limit-modules option. Unfortunately tests start failing on SPARC where Graal (jdk.internal.vm.compiler) is not available. JDK-8191653 [2] reversed 8190975 changes to fix that problem.

This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM only when Graal is used: when it is explicitly specified with -Djvmci.Compiler=graal or in default case and when UseJVMCICompiler is true.

I tested the fix with failed tests from JDK-8190975 which are mostly AppCDS tests in test/runtime/appcds/jigsaw and also test/jdk/java/lang/String/concat/WithSecurityManager.java

I think this fix may break upgradeable status of Graal (for Oracle Labs version of Graal). But it should be fine since it is only used with --limit-modules which is not used by Labs.

Thanks, Vladimir

[1] http://hg.openjdk.java.net/jdk/hs/rev/8deb7919d118 [2] http://hg.openjdk.java.net/jdk/hs/rev/b38d8aadcada

On 29 Nov 2017, at 10:05, Alan Bateman <Alan.Bateman@oracle.com> wrote:

On 28/11/2017 23:51, Vladimir Kozlov wrote:

...

http://cr.openjdk.java.net/~kvn/8191788/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8191788

This is redo of JDK-8190975 [1] fix which added jdk.internal.vm.compiler to tests which have --limit-modules option. Unfortunately tests start failing on SPARC where Graal (jdk.internal.vm.compiler) is not available. JDK-8191653 [2] reversed 8190975 changes to fix that problem.

This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM only when Graal is used: when it is explicitly specified with -Djvmci.Compiler=graal or in default case and when UseJVMCICompiler is true.

I tested the fix with failed tests from JDK-8190975 which are mostly AppCDS tests in test/runtime/appcds/jigsaw and also test/jdk/java/lang/String/concat/WithSecurityManager.java

I think this fix may break upgradeable status of Graal (for Oracle Labs version of Graal). But it should be fine since it is only used with --limit-modules which is not used by Labs. If jdk.internal.vm.compiler is not observable on SPARC then shouldn't the tests have `@requires jdk.internal.vm.compiler` and jtreg will skip the test on that platform?

Just asking as augmenting the value passed to --limit-modules is very strange. It's normal for XX options to augment the set of modules that resolved (+EnableJVMCI implies `--add-modules jdk.internal.vm.ci` for example) but doing this for --limit-modules suggests the VM is doing something to mask an issue with the way that the tests are run.

As much as I understand the issue, I agree. This seems like something that should be addressed in the test(s) instead of in the VM. -Doug

On 29/11/2017 11:26, David Holmes wrote:

The current approach basically prevents anyone using JVMCI from shooting themselves in the foot by setting --limit-modules in a way that excludes the jdk.internal.vm.compiler module. In essence we want to ensure that if using JVMCI then all the requisite pieces will be available. But perhaps we are doing this the wrong way: how do --limit-modules and --add-modules combine? We could add jdk.internal.vm.compiler rather than expanding the limit-set. But the end result would seem the same.

The VM shouldn't augment the value specified to --limit-modules so I think we need to do back to the original issue and find a better solution. Is JDK-8190975 the real issue that we should be looking at? Is it just the two tests listed? In the case of BootAppendTests then it can check if the module is observable before specifying it to --limit-modules when creating the child VM. WithSecurityManager might need additional work or maybe it can drop the use of --limit-modules. As regards using --limit-modules and --add-modules together then it is possible, the details are in JEP 261. -Alan.

On 11/29/17 4:37 AM, Alan Bateman wrote:

On 29/11/2017 11:26, David Holmes wrote:

...

The current approach basically prevents anyone using JVMCI from shooting themselves in the foot by setting --limit-modules in a way that excludes the jdk.internal.vm.compiler module. In essence we want to ensure that if using JVMCI then all the requisite pieces will be available. But perhaps we are doing this the wrong way: how do --limit-modules and --add-modules combine? We could add jdk.internal.vm.compiler rather than expanding the limit-set. But the end result would seem the same.

The VM shouldn't augment the value specified to --limit-modules so I think we need to do back to the original issue and find a better solution. Is JDK-8190975 the real issue that we should be looking at? Is it just the two tests listed? In the case of BootAppendTests then it can check if the module is observable before specifying it to --limit-modules when creating the child VM. WithSecurityManager might need additional work or maybe it can drop the use of --limit-modules.

There are more AppCDS tests which failed too. Originally they we in closed repo and not listed. But now they are in open: http://hg.openjdk.java.net/jdk/hs/file/461e9c898e80/test/hotspot/jtreg/runti... 16 tests.

As regards using --limit-modules and --add-modules together then it is possible, the details are in JEP 261.

I tried to add jdk.internal.vm.compiler to --add-modules instead of --limit-modules. But it fails because it will require to add all Graal's "required" modules too: http://hg.openjdk.java.net/jdk/hs/file/461e9c898e80/src/jdk.internal.vm.comp... If it acceptable I can do that instead. But I insist that we should do that in JVM. I don't want to skip tests on SPARC which have nothing to do with Graal. Thanks, Vladimir

-Alan.

On Nov 29, 2017, at 9:27 AM, Vladimir Kozlov <vladimir.kozlov@oracle.com> wrote:

On 11/29/17 4:37 AM, Alan Bateman wrote:

...

On 29/11/2017 11:26, David Holmes wrote:

...

The current approach basically prevents anyone using JVMCI from shooting themselves in the foot by setting --limit-modules in a way that excludes the jdk.internal.vm.compiler module. In essence we want to ensure that if using JVMCI then all the requisite pieces will be available. But perhaps we are doing this the wrong way: how do --limit-modules and --add-modules combine? We could add jdk.internal.vm.compiler rather than expanding the limit-set. But the end result would seem the same.

The VM shouldn't augment the value specified to --limit-modules so I think we need to do back to the original issue and find a better solution. Is JDK-8190975 the real issue that we should be looking at? Is it just the two tests listed? In the case of BootAppendTests then it can check if the module is observable before specifying it to --limit-modules when creating the child VM. WithSecurityManager might need additional work or maybe it can drop the use of --limit-modules.

There are more AppCDS tests which failed too. Originally they we in closed repo and not listed. But now they are in open:

http://hg.openjdk.java.net/jdk/hs/file/461e9c898e80/test/hotspot/jtreg/runti...

16 tests.

...

As regards using --limit-modules and --add-modules together then it is possible, the details are in JEP 261.

I tried to add jdk.internal.vm.compiler to --add-modules instead of --limit-modules. But it fails because it will require to add all Graal's "required" modules too:

http://hg.openjdk.java.net/jdk/hs/file/461e9c898e80/src/jdk.internal.vm.comp...

If it acceptable I can do that instead.

But I insist that we should do that in JVM. I don't want to skip tests on SPARC which have nothing to do with Graal.

I agree with Vladimir. Skipping these tests on SPARC is not applaudable in this case as they are not Graal specific tests. Thanks, Jiangli

Thanks, Vladimir

...

-Alan.

Yes, it was my limited understanding what --limit-modules is. We should not add modules "under cover" when --limit-modules is used. User should known all required modules *explicitly* to create correct image with jlink based on result of runs with --limit-modules flag. Since it is limited set of tests which use --limit-modules (11 in hotspot, 23 in jdk and few in closed) I will exclude these tests from running with Graal as JIT by adding @require to them: @requires !vm.graal.enabled We may revisit this later in a future when Graal become default JIT compiler (it should be supported on platforms at that time). Thanks, Vladimir On 11/29/17 2:44 PM, mandy chung wrote:

Vladimir and I discussed this offline. java --limit-modules should have the same set of observable modules as running on an image that is created with jlink.  When running with -XX:+UseJVMCICompiler on a runtime image with just java.base, it will fail in the same way as running with java --limit-modules java.base -XX:+UseJVMCICompiler. These tests are written to run on a runtime image with java.base only should be updated to get it run with -XX:+UseJVMCICompiler.  I will take a look and see how we can refactor the tests to support such testing configuration.   In the meantime, Vladimir suggests to exclude these tests when running with -XX:+UseJVMCICompiler (specifying @requires).

Mandy

On 12/13/17 12:16 AM, Vladimir Kozlov wrote:

After several rounds of testing (which found different types of failures with Graal as JIT compiler) I decided to limit these changes for cases when --limit-modules is used:

http://cr.openjdk.java.net/~kvn/8191788/webrev.01/

Looks okay. Since the number of tests with --limit-modules is small, filtering them out when running with Graal as JIT compiler is a reasonable approach for JDK 10.   At some point, we may revisit this and possibly a test library to deal with similar use case - tests with limited modules when running with additional VM flags requiring observable modules.  I will create an issue for it. Mandy

On 13/12/2017 16:47, mandy chung wrote:

On 12/13/17 12:16 AM, Vladimir Kozlov wrote:

...

After several rounds of testing (which found different types of failures with Graal as JIT compiler) I decided to limit these changes for cases when --limit-modules is used:

http://cr.openjdk.java.net/~kvn/8191788/webrev.01/

Looks okay.

Since the number of tests with --limit-modules is small, filtering them out when running with Graal as JIT compiler is a reasonable approach for JDK 10.   At some point, we may revisit this and possibly a test library to deal with similar use case - tests with limited modules when running with additional VM flags requiring observable modules.  I will create an issue for it.

I looked at the webrev too, looks good and much better than modifying the VM to augment the list specified to --limit-modules. -Alan

Hi Alan, On 11/29/17 1:05 AM, Alan Bateman wrote:

On 28/11/2017 23:51, Vladimir Kozlov wrote:

...

http://cr.openjdk.java.net/~kvn/8191788/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8191788

This is redo of JDK-8190975 [1] fix which added jdk.internal.vm.compiler to tests which have --limit-modules option. Unfortunately tests start failing on SPARC where Graal (jdk.internal.vm.compiler) is not available. JDK-8191653 [2] reversed 8190975 changes to fix that problem.

This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM only when Graal is used: when it is explicitly specified with -Djvmci.Compiler=graal or in default case and when UseJVMCICompiler is true.

I tested the fix with failed tests from JDK-8190975 which are mostly AppCDS tests in test/runtime/appcds/jigsaw and also test/jdk/java/lang/String/concat/WithSecurityManager.java

I think this fix may break upgradeable status of Graal (for Oracle Labs version of Graal). But it should be fine since it is only used with --limit-modules which is not used by Labs. If jdk.internal.vm.compiler is not observable on SPARC then shouldn't the tests have `@requires jdk.internal.vm.compiler` and jtreg will skip the test on that platform?

No, these testes (listed above) has nothing to do with Graal. So we can't guard them based on presence of Graal. That was the problem and that is why original 8190975 fix had to be reversed.

Just asking as augmenting the value passed to --limit-modules is very strange. It's normal for XX options to augment the set of modules that resolved (+EnableJVMCI implies `--add-modules jdk.internal.vm.ci` for example) but doing this for --limit-modules suggests the VM is doing something to mask an issue with the way that the tests are run.

Just reminder - Graal is upgradeable module [1]. We can't do for it what we do for JVMCI as you pointed. [1] https://bugs.openjdk.java.net/browse/JDK-8177845 Thanks, Vladimir

-Alan