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

[harold seigel]

Hi Claes,

The changes look good.

Could you run the RBT hs nightly tests against the Linux-x64 platform 
before pushing this change?

Thanks, Harold


On 1/6/2017 9:35 AM, Claes Redestad wrote:
> 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
>>