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
>