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

[harold seigel]

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