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

Claes Redestad claes.redestad at oracle.com
Sun Oct 12 02:09:30 UTC 2014

On 2014-10-11 02:31, Mandy Chung wrote:
> 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.

Map misses the atomicity requirement of putIfAbsent, ConcurrentMap is OK 
but leaves some
related questions open (why we can't add a null value, specifically). 
I'm glad it was brought up
and discussed and will use ConcurrentHashMap for private fields unless 
there's a strong
preference otherwise.

> 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.

Right, I was I think synchronization can still be avoided by throwing IAE if
putIfAbsent doesn't return null:

         if (packages.putIfAbsent(name, pkg) != null) {
             throw new IllegalArgumentException(name);
         return pkg;

> 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.

Yes, an unlikely race.

> 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 I'll take your suggestion below and ensure cachedManifest and 
it's getManifest()
never evaluate to null, which makes for a cleaner patch. There is some 
code duplication
here with URLClassLoader#definePackage. Future cleanup?

It would seem the ClassLoader argument in this ctor is always called 
with null. Remove?

> 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);

Yes. Looked a bit dangerous, but it seems we still maintain the 
necessary guarantees.

> 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.

Sure! 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:


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?

> 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)?

I'll take a look at the current test coverage and give it some thought.



> Mandy

More information about the core-libs-dev mailing list