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

Claes Redestad claes.redestad at oracle.com
Mon Jan 9 12:16:51 UTC 2017


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?

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


More information about the jigsaw-dev mailing list