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

[serguei.spitsyn at oracle.com]

On 1/9/17 04:16, Claes Redestad wrote:
> Hi Serguei,
>
> On 2017-01-09 09:11, serguei.spitsyn at oracle.com wrote:
>> Hi Claes,
>>
>> It looks pretty good.
>
> thanks,
>
>>
>> One question on the following fragment:
>>
>> http://cr.openjdk.java.net/~redestad/8171855/jdk.04/src/java.base/share/native/libjava/Module.c.udiff.html 
>>
>>
>>
>> + int valid = 1;
>> + for (idx = 0; idx < num_packages; idx++) {
>> + jstring pkg = (*env)->GetObjectArrayElement(env, packages, idx);
>> + pkgs[idx] = GetInternalPackageName(env, pkg, NULL, 0);
>> + if (pkgs[idx] == NULL) {
>> + valid = 0;
>> + break;
>> + }
>> + }
>> +
>> + if (valid != 0) {
>> + JVM_DefineModule(env, module, is_open, version, location,
>> + (const char* const*)pkgs, num_packages);
>> + }
>> + }
>>
>>
>>   It looks like the module won't be defined if there was an OOM in
>> processing one of the package names.
>>   The malloc is used only if the supplied buffer size is not enough.
>>   Would it make sense to increase the buffer size from 128 to 256 (or
>> even 512) at the lines:
>>
>> 119 char buf[128];      140 char buf[128];
>>     161 char buf[128];      181 char buf[128];
>>
>>
>>   Nit: it is also better to use a named constant instead.
>
> The 128 value was taken from similar code in Class.c, and since package
> names are always shorter I assumed there was no real need to evaluate
> other heuristics.
>
> How about re-visiting this for an enhancement in 10 and evaluate if
> a different value and a freshly introduced constant makes sense?

Sure.

Thanks,
Serguei

>
> Thanks!
>
> /Claes
>
>>
>> Thanks, Serguei On 1/6/17 06:23, Claes Redestad wrote:
>>> On 2017-01-06 15:07, Alan Bateman wrote:
>>>> On 06/01/2017 13:18, Claes Redestad wrote:
>>>>> : Updated jdk webrev in-place:
>>>>> http://cr.openjdk.java.net/~redestad/8171855/jdk.04/
>>>> I looked through the latest webrev and it looks quite good. Part of
>>>> me regrets introducing JNI code of course but this is startup
>>>> motivated. In Module.c then I assume GetInternalPackageName should be
>>>> static getInternalPackageName as the scope is just this file (no need
>>>> for JNIEXPORT).
>>> Done, will update in place.
>>>> Do we have tests that use package names > 128 to test cases where the
>>>> buffer on the stack not large enough?
>>> I'm not sure; the malloc'ing path is exercised by DefineModule, but it
>>> would make sense to add a sanity test to each method I guess. /Claes