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

[Claes Redestad]

On 10/13/2014 04:18 AM, David Holmes wrote:
> 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.

Right, this would be a throughput-footprint tradeoff: adding more CHM 
entries versus taking a small penalty for having to potentially look 
things up in all the ancestral classloaders + Packages.pkgs.

I believe the caching was put in place mostly to avoid a chain of 
relatively expensive synchronization locks with potential for deadlocks, 
not to solve an actual throughput problem in uncontended cases. There 
might be a break-off somewhere due to chaining classloaders, but I've 
not been able to construct a benchmark where this is the case. I could 
swing either way on this, but with a trivial lookup cost even in the 
worst case I generally think we should favor footprint here and defer 
caching-for-performance to the application if ever necessary.

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

Very interesting, but since we're not changing behavior and the issue of 
classloading order Peter points out can't be resolved by adding 
atomicity guarantees, I feel this is out of scope for this cleanup. 
Should we file a new bug to examine this?

/Claes

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