Re: Patch: FYI: More efficient ResourceBundle calls

[Dalibor Topic]

Bryce McKinlay wrote:
> Mark Wielaard wrote:
>
> > On Tue, 2004-06-15 at 16:56, Bryce McKinlay wrote:
> >  
> >
> > > I'm checking in this patch from libgcj. It changes various
> > > ResourceBundle.getBundle() calls to use the 3-argument form which
> > > includes a ClassLoader parameter. This call is  more efficient because
> > > it means getBundle() does not have to walk the stack to find the
> > > calling classloader. This should speed up some Date/Calendar
> > > operations which are currently quite slow due to repeated calling
> > > classloader checks. Other VMs may implement the classloader check
> > > faster than libgcj does currently, however it will presumably always
> > > be faster to pass the classloader explicitly - so please use the
> > > 3-argument form when adding new getBundle() calls.
> > >   
> >
> >
> > This looks a bit bogus to me.
> >
> >  
> >
> > > -    return ResourceBundle.getBundle(bundleName, locale);
> > > +    return ResourceBundle.getBundle(bundleName, locale,
> > > +      Calendar.class.getClassLoader());
> > >   
> >
> >
> > In every case you call getClassLoader() on a bootstrap class. So you
> > already know that it will be null. So you could have just passed in null
> > as loader. But ResourceBundle.getBundle() is documented to throw a
> > NullPointerException when any of its arguments is null. It seems we have
> > a bug because we don't throw a NullPointerException (we used to throw it
> > when we used HashTable, but we switched to HashMap which allows null
> > values), but the kaffe implementation does (IMHO correctly) throw a
> > NullPointerException.
> >  
>
> OK, in libgcj, getClassLoader() never returns null, but the spec does 
> say that implementations may use null to represent the bootstrap 
> classloader. Since the classloader argument to getBundle() may not be 
> null, so we have a problem.
>
> I think the solution here is to pass the system class loader. This 
> should always be correct for these bootstrap classes. It doesn't solve 
> the performance issues for cases where a security manager is present, 
> since a check will be performed by the getSystemClassLoader call - 
> however its better than what we had before and certainly better than my 
> incorrect patch ;-)

Ah, the curse of getting microoptimizations right ;)

I think the patch is still wrong, in cases where one uses a Calendar 
class that's not loaded via the system class loader (like the bootstrap 
class loader, extension class loader, etc [1] :). I think you have to use

ClassLoader loader = Calendar.class.getClassLoader();
if (loader == null)
{
        loader = ClassLoader.getSystemClassLoader();
}
return ResourceBundle.getBundle(bundleName, locale, loader);

And by now, I think it's sufficiently complicated (5 lines instead of 
one) that just reverting it to what it was before is a better idea wrt 
maintenance :)

cheers,
dalibor topic

[1] http://www.javageeks.com/Papers/ClassForName/ClassForName.pdf