RFR: 8171855: Move package name transformations during module bootstrap into native code

[Claes Redestad]

Lois, Mandy,

On 2017-01-05 22:19, Lois Foltan wrote:
 >
 > On 1/5/2017 11:47 AM, Claes Redestad wrote:
 >> Hi,
 >>
 >> after a round of review comments I've now reworked this to do the
 >> transformations in the JNI layer rather than inside the VM, with
 >> similar - if not better - results.
 >>
 >> Webrevs:
 >> http://cr.openjdk.java.net/~redestad/8171855/hotspot.03/
 >> http://cr.openjdk.java.net/~redestad/8171855/jdk.03/
 >>
 >> Testing: RBT run in progress, all runtime/modules tests pass locally
 >
 > Hi Claes,
 >
 > Thank you for reworking this.  I think it looks good.  A couple of
 > comments in hotspot/src/share/vm/classfile/modules.cpp
 >
 > modules.cpp/Modules::define_module()
 >   line #278, I think it is important for the VM to validate the packages
 > and num_packages parameters.
 >     An IllegalArgumentException should be thrown if:
 >     - num_packages is not >= 0
 >     - if packages == NULL, then num_packages should be equal to 0,
 >       this will protect against erroneously entering the for loop at
 > line #292 based solely on num_packages

Done.

 >   line #291 - Ideally, if num_packages is 0, there is no sense in
 > allocating pkg_list
 >
 > Thanks,
 > Lois

There's plenty of code which assumes pkg_list is always allocated (even
when num_packages is 0), so if you don't mind I'd like to defer this
cleanup?

There's only a handful of modules that doesn't define any packages, so
I don't think it'll matter that much for startup.

Thinking out loud: I'm more concerned about the fact we're scanning the
growing pkg_list for duplicates on every insertion, which is
effectively quadratic.  It seems we could just as well skip this check
and let the error be caught by the dupl_pkg_index checking later on
(then scan the packages for duplicates in the error handling code to
distinguish error messages).  Would mean we could do away with the
GrowableArray too..

On 2017-01-05 22:24, Mandy Chung wrote:
>
> Happy to know the performance gain is comparable when pushing down the conversion to internal form in native instead of doing it in the VM.  This is good work.

Thanks!

>
> src/java.base/share/native/libjava/Module.c
>   This can be refactored e.g. adding a new function GetInternalPackageName that takes a jstring and returns const char*.
>
>   GetStringUTFChars will return a copy of the utf-8 chars. That is an alternative to malloc, GetStringUTFLength, GetStringLength, GetStringUTFRegion calls.  Use ReleaseStringUTFChars to free the copy after use.

GetStringUTFChars returns a const char*, so wouldn't work as
we're mutating.  I'll do as you suggest and refactor some common code
to a new method GetInternalPackageName to reduce the boiler plate,
though.

>
>   Nit: it may be clearer to rename pkgs_len to num_pkgs

Done.

>
> src/share/vm/classfile/modules.cpp
>   49 static bool verify_module_name(const char *module_name) {
>
> To be consistent with the convention in this file: const char*

New webrevs:
http://cr.openjdk.java.net/~redestad/8171855/hotspot.04/
http://cr.openjdk.java.net/~redestad/8171855/jdk.04/

In addition to fixing comments I couldn't resist applying the technique
used in Class.c to get rid of some mallocs in places where we can use a
stack allocated char buf[128] instead, which seems to have a tiny, but
measurable effect.

Thanks!

/Claes