RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package

[Peter Levart]

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