On 10/12/2014 04:09 AM, Claes Redestad wrote:
Taking in all these suggestions as well as realizing a race could
cause different Package
to return from subsequent calls to Package.defineSystemPackage brings
me to this:
http://cr.openjdk.java.net/~redestad/8060130/webrev.03/
Hi Claes,
Just some nits...
- I think you're still using Map as the type of 'pkgs' and 'manifests'
static fields in Package although they are CHMs at runtime.
- To avoid unnecessary (multiple) reads from a volatile field, the
following code in CachedManifest:
631 public Manifest getManifest() {
632 resolveManifest();
633 return manifest;
634 }
635
636 private void resolveManifest() {
637 if (manifest != null || url == null) {
638 return;
639 }
640 synchronized(this) {
641 if (manifest == null) {
642 manifest = AccessController.doPrivileged(new
PrivilegedAction<Manifest>() {
643 public Manifest run() {
644 return loadManifest(fileName);
645 }
646 });
647 }
648 }
649 }
could be written as:
public Manifest getManifest() {
if (url == null) return null;
Manifest m = manifest;
if (m != null) return m;
synchronized (this) {
if ((m = manifest) != null) return m;
manifest = m = AccessControler.doPrivileged(....);
return m;
}
}
- It would be better to move loadManifest() into the CachedManifest
class since it's only used there as a helper method (to avoid creating
access bridges or having to declare it package-private instead) - well
no, this doesn't help, since it's called from an anonymous inner
PrivilegedAction subclass. What about just inlining it into the
PrivilegeAction's run() method?
I also wonder if the lazy loading of Manifest in CachedManifest is
necessary. If you look at the only usage of CachedManifest.getManifest()
method (in Package.defineSystemPackage()):
CachedManifest cachedManifest = createCachedManifest(fn);
pkgs.putIfAbsent(name, new Package(name,
cachedManifest.getManifest(),
cachedManifest.getURL()));
...you can see that getManifest() is called immediately after obtaining
the CacheManifest. So there's no need for lazy loading. Loading the
Manifest in CachedManifest constructor would be just fine.
Now... How about a slightly alternative approach: instead of caching
Manifests we could create
and cache a Package - call it a prototype - then add a private
constructor taking the
package name and the "prototype" Package. The Package objects should
come with a
smaller footprint and have the added benefit of being effectively
immutable. Does that
sound like an improvement?
Or, call the prototype a 'CachedManifest' and Package just delegate
methods to it. Well...
A noble goal, but this space optimization would only pertain to system
packages if changed only in ClassLoader/Package. You would still have to
have "fat" Packages because other ClassLoaders define their Packages
with the other Package constructor that doesn't take the Manifest file
name (which would be used as a key to obtain a prototype), but use their
own Manifest parsing (see URLClassLoader.definePackage()) ... There are
just a couple of system packages. So you would have to also change the
API between individual ClassLoader(s) and ClassLoader/Package (see
ClassLoader.definePackage()) to optimize other non-system packages which
outnumber system packages. There are not so many packages after all
(compared to Class(es)).
Regards, Peter
