Christian, Sergey,
I've modified the test per your findings. Now it is one java file
and one C file only.
Please, find the updated review at:
Webrev: http://cr.openjdk.java.net/~akulyakh/8153978_6/
Thank you very much for your help.
Best regards,
Alexander
----- Original Message -----
From: serguei.spit...@oracle.com
To: christian.tornqv...@oracle.com, alexander.kulyakh...@oracle.com
Cc: serviceability-dev@openjdk.java.net
Sent: Thursday, July 21, 2016 6:39:21 PM GMT +03:00 Iraq
Subject: Re: RFR:8153978:New test to verify the modules info as
returned by the JVMTI
On 7/21/16 08:29, Christian Tornqvist wrote:
Hi Alexander,
>The JVMTI always reports 3 unnamed modules: the boot module, the
system module and the application module.
>The Java API does not report any unnamed modules.
I’ll leave this up to you if this is something that we need to
verify or not, the code for doing this is also overcomplicated
and can be reduced to a simple assertGTE.
The rule is that there is one unnamed module per a class loader.
The options are: to test this rule or remove the check.
For simplicity is better to remove this check as unclear.
Thanks,
Serguei
>This should be doable without using JAR's and custom loaders by
using Layer.defineModules(), see the examples in
jdk/test/java/lang/reflect/Layer/BasicLayerTest.java
>The test has been written from the user perspective. The user loads
a new module in the form of jar using the
ModuleLoader.loadModule() API. Then the test checks that JVMTI
does return the info about that loaded module.
>Probably, defining the module using Layer.defineModules would not be the
same as loading the module using ModuleLoader.loadModule(),
since the JVMTI GetAllModules() returns the info about all the
currently loaded modules.
>As the JVMTI spec says: "GetAllModules: Return an array of all
modules loaded in the virtual machine.", it does not mention
defining modules.
There are several ways to get modules loaded/defined, the
Layer.defineModules is part of the official Java API and is one
of them. It doesn’t matter to JVMTI if they come from JAR files
on disk or if they’re defined using a Java API, so I suggest you
go with Layer.defineModules.
Thanks,
Christian
*From:*Alexander Kulyakhtin
[mailto:alexander.kulyakh...@oracle.com]
*Sent:* Thursday, July 21, 2016 10:04 AM
*To:* Serguei Vladimirovich Spitsyn
<serguei.spit...@oracle.com>; christian.tornqv...@oracle.com
*Cc:* serviceability-dev@openjdk.java.net
*Subject:* Re: RFR:8153978:New test to verify the modules info
as returned by the JVMTI
Christian,
Thank you very much for your comments. I have some concerns
about the proposed changes:
@45 & @67
Why is this check needed? Why are there least 3 unnamed modules?
The JVMTI always reports 3 unnamed modules: the boot module, the
system module and the application module.
The Java API does not report any unnamed modules.
@54
This should be doable without using JAR's and custom loaders by
using Layer.defineModules(), see the examples in
jdk/test/java/lang/reflect/Layer/BasicLayerTest.java
The test has been written from the user perspective. The user
loads a new module in the form of jar using the
ModuleLoader.loadModule() API. Then the test checks that JVMTI
does return the info about that loaded module.
Probably, defining the module using Layer.defineModules would
not be the same as loading the module using
ModuleLoader.loadModule(), since the JVMTI GetAllModules()
returns the info about all the currently loaded modules.
As the JVMTI spec says: "GetAllModules: Return an array of all
modules loaded in the virtual machine.", it does not mention
defining modules.
Could you, please, clarify these points for me so I fix the test
appropriately?
Best regards,
Alexander
----- Original Message -----
From: christian.tornqv...@oracle.com
<mailto:christian.tornqv...@oracle.com>
To: alexander.kulyakh...@oracle.com
<mailto:alexander.kulyakh...@oracle.com>,
serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>
Sent: Wednesday, July 20, 2016 8:11:14 PM GMT +03:00 Iraq
Subject: RE: RFR:8153978:New test to verify the modules info as
returned by the JVMTI
Hi Alexander,
This test is unnecessarily complicated, it could be simplified a
lot.
JvmtiGetAllModulesTest.java
Move getModulesNative() into JvmtiGetAllModulesTest.java and
have it return a Set<Module> to be able to use equals later
@27 * @compile JvmtiGetAllModulesTest.java
No need for this, jtreg will compile it for you
@45 & @67
Why is this check needed? Why are there least 3 unnamed modules?
@50
Change this to: assertTrue(Layer.boot().equals(getModulesNative()));
@54
This should be doable without using JAR's and custom loaders by
using Layer.defineModules(), see the examples in
jdk/test/java/lang/reflect/Layer/BasicLayerTest.java
@65
Change this to an assertTrue using the layer containing the new
module, similar to the change @50
@73
No need for this method
@81
Change this method to use the Layer.defineModules() method to
define a module instead, this eliminates the need for external
JAR's
@98
No need for this method
If you use Layer.defineModules(), the following files can be
removed:
JarBuilder.java
JavaModulesInfo.java
JvmtiModulesInfo.java
ModuleLoader.java
ModulesInfo.java
module-info.java
Thanks,
Christian
*From:*serviceability-dev
[mailto:serviceability-dev-boun...@openjdk.java.net] *On Behalf
Of *serguei.spit...@oracle.com
*Sent:* Monday, May 2, 2016 6:06 PM
*To:* Alexander Kulyakhtin <alexander.kulyakh...@oracle.com>;
Serviceability-Dev <serviceability-dev@openjdk.java.net>
*Subject:* Re: RFR:8153978:New test to verify the modules info
as returned by the JVMTI
Hi Alexander,
Could you, fix a couple of minor issues?
test/serviceability/jvmti/GetModulesInfo/JvmtiGetAllModulesTest.java
58 for(Module mod : my.modules()) {
59 if(!jvmtiModules.contains(mod)) {
A space is missed after the 'for' and 'if' keywords.
test/serviceability/jvmti/GetModulesInfo/ModulesInfo.java.
31 boolean compareExcludingUnnamed(ModulesInfo other) {
I'd suggest to call it compareNamed.
Otherwise, the new test looks great.
Thanks a lot for taking care about it!
Thanks,
Serguei
On 4/29/16 06:12, Alexander Kulyakhtin wrote:
Hi,
Could you, please, review these test-only changes (adding a new test).
CR:https://bugs.openjdk.java.net/browse/JDK-8153978 "New test to verify the
modules info as returned by the JVMTI"
Webrev:http://cr.openjdk.java.net/~akulyakh/8153978_01/
<http://cr.openjdk.java.net/%7Eakulyakh/8153978_01/>
The new test verifies that JVMTI returns the correct info about the
modules loaded at the application startup.
It also verifies that the returned info is consistent with the same
info returned by the Java API.
It then loads a new named module and checks the correctness of the
JVMTI info again.
Due to a tools
issuehttps://bugs.openjdk.java.net/browse/CODETOOLS-7901662 the test can only
be pushed in when the updated jtreg is released.
The test passes fine with the nightly jtreg build, containing the
CODETOOLS-7901662 fix.
Best regards,
Alexander
