Alan,
Thanks for the comments and suggestions!
On 12/10/15 04:04, 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.
Fixed in both JDI and JDWP.
2. In the Module command then the description would be clearer as "...
that this reference type ...".
Fixed.
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.
The empty string is returned in the implementation.
Would it be Ok to update the jdwp spec with this?
4. Also in the Module command set then the CanRead targetModule should
be "source" (not target) to get consistency with the runtime API.
Fixed.
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, ...
Sure.
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.
Ok, will try it.
ReferenceType::module looks right, we'll just need to iterate on the
javadoc.
Ok.
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.
Ok.
In ReferenceTypeImpl then I assume isModuleCached is not needed.
Not sure, I understand this. Why?
It seems there is some confusion here.
This flag is similar to the flag isClassLoaderCached.
Maybe you have this to the JDWP command when connected to a target VM
that is JDK 8 or older?
No.
For the older JDK the UnsupportedOperationException is expected to be
thrown.
VirtualMachineImpl - this might be the time to change to 9 rather than
1.9.
Fixed all places in the JDI update.
ModuleImpl - I assume ref can be final.
Fixed.
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.
I'll ask Dmitry as he covers the SA.
He had some plans on the Jigsaw update.
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.
Yes.
I'd like to add more sophisticated self-checks to the existing test.
More negative tests ...
Good JDWP and JVMTI tests are needed as well.
Thanks,
Serguei
-Alan
