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

[Mandy Chung]

On 10/10/2014 8:10 AM, Claes Redestad wrote:
> Hi all,
>
> please review this patch which attempts to clean up synchronization 
> and improve scalability when
> defining and getting java.lang.Package objects.

I agree with David that getting Package objects are not performance
critical. On the other hand, the code defining/getting Packages is
old and deserves some cleanup especially the synchronization part.

If you run helloworld program, how does that change the list of loaded
classes besides the new CachedManifest class?

>
> webrev: http://cr.openjdk.java.net/~redestad/8060130/webrev.02/

ClassLoader.java
line 272: can you change the declared type as Map.

definePackage throws IAE if there exists an existing package either
in this class loader or one of its ancestors.
   - this change would not catch if two definePackage calls to define
   a package of the same name but with different spec version, title, etc
   concurrently.

   You may not be able to avoid synchronizing on packages for this case.

move line 1623 to 1630 so that the declaration of map is closer to
the assignment.

Package.java

line 557 there is a possibility that new Package[pkgs.size()] is not
big enough and a new array would be created.  As this method is not
popularly used, it's okay if another array is created.

line 563 and 565 can be merged

line 570-575: do you think you can modify the private
   Package(String name, Manifest man, URL url, ClassLoader loader) constructor
to take null Manifest and null url so that these lines can be folded into
  
pkg = new Package(name,  cachedManifest.getManifest(),  cachedManifest.getURL(), null);

I think CachedManifest class and the createCachedManifest method need
some work.  Perhaps we can have the CachedManifest constructor to
obtain the URL.

Each invalid fn will have one instance instead of NO_MANIFEST singleton
but that should not happen as fn is the filename where the classes
loaded from the bootclasspath.  CachedManifest.url can then become final.

line 587-601 would not be needed. Can we avoid line 606 and write
the createCachedManifest method this way:
    if (!manifests.containsKey(fn)) {
        manifests.putIfAbsent(fn, new CachedManifest(fn));
    }
    return manifests.get(fn);

You may be able to further simplify CachedManifest and remove the resolved
field by storing an empty Manifest when loadManifest returns null.
That may help the private Package constructor not require any change
to merge line 570-575 as my comment noted above.

You will need to check there is any test to verify Package created with
and without manifest.  Do you mind making this change and tests (I realize
it might be out of scope of this performance improvement you initially
anticipated)?

Mandy