|
From: | Dalibor Topic |
Subject: | Re: Patch: FYI: More efficient ResourceBundle calls |
Date: | Wed, 16 Jun 2004 12:34:04 +0200 |
User-agent: | Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040608 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |