Thanks, Harold!
Serguei
On 6/28/16 10:45, harold seigel wrote:
Hi Serguei,
It looks good!
Thanks, Harold
On 6/28/2016 12:37 PM, [email protected] wrote:
Hi Harold,
Thank you again for the comments!
This is the updated webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.5/
Thanks,
Serguei
On 6/28/16 07:32, harold seigel wrote:
Hi Serguei,
Looks good.
1. In modules.cpp, the first assert in get_named_module() has the
wrong function name:
825 assert(ModuleEntryTable::javabase_defined(),
826 "Attempt to call *get_module_by_package_name* before java.base
is defined");
2. Also, is a check needed to ensure that package_str is not null?
3. Is the ResourceMark(THREAD) needed at line 824?
Thanks, Harold
On 6/28/2016 4:54 AM, [email protected] wrote:
Hi David,
Thank you for the comments!
I agree with you.
Updated Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.4/
Thanks,
Serguei
On 6/27/16 22:06, David Holmes wrote:
Hi Serguei,
Overall this looks a lot clearer/simpler.
On 28/06/2016 2:20 PM, [email protected] wrote:
On 6/27/16 21:08, [email protected] wrote:
Please, review the Jigsaw fix for the enhancement:
https://bugs.openjdk.java.net/browse/JDK-8159145
The Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.3/
Are there intended to be other callers of
Module::get_named_module? If not it seems odd to go to all the
trouble of throwing exceptions, just to have the caller clear them
and return an error code. Or you could move all that argument
checking code into the JVMTI function directly - if called
internally you would just assert that arguments are as expected.
src/share/vm/prims/jvmti.xml
+ otherwise the <code>NULL</code> is returned.
Delete "the".
Otherwise all looks good to me.
The Jdk webrev is the same:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/
Sorry, the Jdk webrev changed as well:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk3/
Looks fine.
Thanks,
David
Thanks,
Serguei
Summary:
This is the review round #3.
Alan suggested to replace the function GetModuleByPackageName
with
the GetNamedModule.
New function will return NULL if the package is not in a module
defined to the class loader.
It simplifies the API and makes it easier to specify.
JVM TI agents that instrument code in named modules need the
Module
at class load time.
One question that came from the function semantics change.
I had to implement the Modules::get_named_module() from scratch
independently of the existing
Modules::get_module_by_package_name() and Modules::get_module().
The issue is that the Modules::get_module() can return the
unnamed
module whereas the
JVMTI helper Modules::get_named_module() should return NULL
instead
of the unnamed module.
Please, let me know if it is Ok or if you have better ideas
how to
share the code.
This is the Summary from review round #1:
One way to do this is by introducing a new ClassFileLoadHook that
takes an additional parameter but this approach is disruptive.
The alternative option is a JVM TI function that maps a
classloader
+ package name to a module.
We were initially not confident with this approach so we
introduced
it as JVM function JVM_GetModuleByPackageName.
Based on experience to date then this approach seems okay and so
this function needs to be promoted to a JVMTI function.
It includes new jtreg test with native JVMTI agent.
Testing:
Run newly developed jtreg test.
Thanks,
Serguei
