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

[David Holmes]

Hi Claes,

Looking at version three ...

You seemed to have changed the caching strategy for Packages in the 
classloader. Previously a Package defined by a parent or the system 
would be updated in the current loader's Package map; but now you leave 
them separate so future lookups will always have to traverse the 
hierarchy. This doesn't seem like a performance gain.

Looking at definePackage it seems both old and new code have serious 
race conditions due to a lack of atomicity when checking the 
parent/system packages. A package of the same name could be defined in 
the parent/system after definePackage has called getPackage - and we 
then end up with two same named Packages in different loaders.

No comment on the manifest caching aspect - I'm not familiar enough with 
the existing code.

On 12/10/2014 12:09 PM, Claes Redestad wrote:
> 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
>>
>