Hi Serguei,
Looks good. Just a few comments.
1. Should jni_GetAllModules() contain the DTrace macros that other JNI
entries have?
2. Can you add a similar GetAllModules() function to jniCheck.cpp ?
Thanks, Harold
On 12/10/2015 7:04 AM, Alan Bateman wrote:
On 10/12/2015 08:03, serguei.spit...@oracle.com wrote:
Jdk webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8049365-Jigsaw-jdk.0/
Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8049365-Jigsaw-hs.0/
Summary:
It is expected that the JDI and JDWP update for modules will have
several iterations.
This is the initial one, and it introduces a very minimal
functionality.
The main purpose of this preliminary review is to make sure the JDI
and JDWP update
for modules goes in a right direction and has nothing obviously wrong.
I went through the changes to the JDWP spec + additions to jdwpgen in
this iteration and it looks quite good. A few comments:
1. I don't know if JEP 223 considered the JDWP version but I will
assume this will move to 9 instead of 1.9.
2. In the Module command then the description would be clearer as "...
that this reference type ...".
3. In the Module command set then we'll need to decide the reply to
the Name command for the case that the module is an unnamed module.
There is also an open issue for the runtime API too.
4. Also in the Module command set then the CanRead targetModule should
be "source" (not target) to get consistency with the runtime API.
As you mentioned in your mail, I think the GetAllModules will need to
move to JVM TI so that it's consistent with GetAllClasses,
GetAllThreads, ...
I also went through the JDI changes. The type hierarchy in JDI is
quite deep and is has a convention for naming that we need to follow
too. Looking at it now then it looks like it should be ModuleReference
extends ObjectReference. That would give us consistency with other
mirrors that are object references (ThreadReference and
ClassLoaderReference for example). We don't rev JDI often enough to
keep the hierarchy and naming convention in our heads so good to have
more eyes on this.
ReferenceType::module looks right, we'll just need to iterate on the
javadoc.
VirtualMachine::allModules looks right. Note that the TRACE_* fields
are public and part of the JDI API so we'll need to decide whether
TRACE_MODULES is important or not.
In ReferenceTypeImpl then I assume isModuleCached is not needed. Maybe
you have this to the JDWP command when connected to a target VM that
is JDK 8 or older?
VirtualMachineImpl - this might be the time to change to 9 rather than
1.9.
ModuleImpl - I assume ref can be final.
Have you thought about SA yet? I can't recall if it is compiled with
the boot JDK or will be compiled against the newly built jdk.jdi
module. If the later then I assume that SA will need updates. If the
former then I assume we will have issues with boot cycle builds.
Tests. I see you have a basic test but I assume we will need to find
time to develop a lot more tests for this update.
-Alan
