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

[Claes Redestad]

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.

Ok

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

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

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.

Thanks!

/Claes

>
> Mandy
>