[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8171855: Move package name transformations during module bootstrap into native code
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2017-01-09 19:20:59
Message-ID: 38b7db2a-824b-1301-50af-01eb51a1bbfd () oracle ! com
[Download RAW message or body]

On 1/9/17 04:16, Claes Redestad wrote:
> Hi Serguei,
> 
> On 2017-01-09 09:11, serguei.spitsyn@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


[prev in list] [next in list] [prev in thread] [next in thread]