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

[Claes Redestad]

Hi Peter,

On 2017-01-06 09:46, Peter Levart wrote:
> Hi Claes,
>
> On 01/06/2017 03:36 AM, Claes Redestad wrote:
>>
>> New webrevs:
>> http://cr.openjdk.java.net/~redestad/8171855/hotspot.04/
>> http://cr.openjdk.java.net/~redestad/8171855/jdk.04/
>
> In Module.c, it seems you applied the "techinique" of conditional
> allocation (lines 49...57), but forgot to remove the old code (58...62):
>
>   49     if (len >= buf_size) {
>   50         utf_str = malloc(len + 1);
>   51         if (utf_str == NULL) {
>   52             JNU_ThrowOutOfMemoryError(env, NULL);
>   53             return NULL;
>   54         }
>   55     } else {
>   56         utf_str = buf;
>   57     }
>   58     utf_str = malloc(len + 1);
>   59     if (utf_str == NULL) {
>   60         JNU_ThrowOutOfMemoryError(env, NULL);
>   61         return NULL;
>   62     }

Ouch, this must have slipped back in during some final, late night
edits.  Thanks for spotting it!

>
> Also, in line 84, you allocate pkgs array only when num_packages != 0:
>
>   84     if (num_packages != 0 && (pkgs = calloc(num_packages,
> sizeof(char*))) == NULL) {
>
> ...but you free it unconditionally (does free(NULL) work as no-op?):

It appears the C specification has said for some time that free(NULL) is
and should be a no-op, but that there might be (old) runtimes that
could crash when you do it.  As I don't know if any of the OpenJDK
platforms might be affected or not I've added some checking.

>
>  110     free(pkgs);
>
>
> In multiple methods of Module.c, you repeatedly use the following idiom:
>
>  131     pkg_name = GetInternalPackageName(env, pkg, buf,
> (jsize)sizeof(buf));
>  132     if (pkg_name == NULL) {
>  133         JNU_ThrowOutOfMemoryError(env, NULL);
>  134     } else {
>
> ...but there's no need to call JNU_ThrowOutOfMemoryError if
> GetInternalPackageName returns NULL since GetInternalPackageName already
> does it.

Yes, this allows for a nice cleanup of that code.

Updated jdk webrev in-place:

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

Thanks!

/Claes

>
>
> Otherwise the jdk part looks good.
>
>
> Regards, Peter
>
>>
>> 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
>