RFR:8153978:New test to verify the modules info as returned by the JVMTI
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Jul 21 18:35:43 UTC 2016
Hi Alexander,
JvmtiGetAllModulesTest.java
It looks pretty good but it would be nice if there is any chance to
simplify even more.
However, I can't suggest anything at the moment.
libJvmtiGetAllModulesTest.c
Unneeded indent for all lines.
Otherwise, it is good.
Thanks,
Serguei
On 7/21/16 10:14, Alexander Kulyakhtin wrote:
> 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.spitsyn at oracle.com
> To: christian.tornqvist at oracle.com, alexander.kulyakhtin at oracle.com
> Cc: serviceability-dev at 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.kulyakhtin at oracle.com]
> *Sent:* Thursday, July 21, 2016 10:04 AM
> *To:* Serguei Vladimirovich Spitsyn <serguei.spitsyn at oracle.com>;
> christian.tornqvist at oracle.com
> *Cc:* serviceability-dev at 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.tornqvist at oracle.com
> <mailto:christian.tornqvist at oracle.com>
> To: alexander.kulyakhtin at oracle.com
> <mailto:alexander.kulyakhtin at oracle.com>,
> serviceability-dev at openjdk.java.net
> <mailto:serviceability-dev at 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-bounces at openjdk.java.net] *On Behalf Of
> *serguei.spitsyn at oracle.com <mailto:serguei.spitsyn at oracle.com>
> *Sent:* Monday, May 2, 2016 6:06 PM
> *To:* Alexander Kulyakhtin <alexander.kulyakhtin at oracle.com>;
> Serviceability-Dev <serviceability-dev at openjdk.java.net
> <mailto:serviceability-dev at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160721/587b4a2b/attachment-0001.html>
More information about the serviceability-dev
mailing list