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

[Claes Redestad]

Hi Harold,

sure, I've updated the patch in-place with this small improvement and
submitted a new set of tests.

/Claes

On 2017-01-06 14:23, harold seigel wrote:

Hi Claes,

One small nit.  In these lines in modules.cpp, the length of
package_name at line 305 could be saved in a local and then strncpy()
could be used at line 306.  Also, the saved length could be used in line
310 instead of strlen(pkg_name).  A similar change would also apply
around line 732.

305 char* pkg_name = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, char,
strlen(package_name));
306 strcpy(pkg_name, package_name);
307 StringUtils::replace_no_expand(pkg_name, "/", ".");
 308       const char* msg_text1 = "Class loader (instance of): ";
 309       const char* msg_text2 = " tried to define prohibited package
name: ";
310 size_t len = strlen(msg_text1) + strlen(class_loader_name) +
strlen(msg_text2) + strlen(pkg_name) + 1;

Thanks, Harold

On 1/5/2017 9:36 PM, Claes Redestad wrote:

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