RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Peter Levart
peter.levart at gmail.com
Mon Oct 13 09:04:49 UTC 2014
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.
>
> 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.
Hmm...
It's hard to come up with a consistent behaviour which would prevent
parent / child ClassLoaders from defining classes in the same package.
You can't prevent child prom defining a package before you know that
parent has a package with the same name and you cant prevent a parent
from defining a package if child has already defined a package with the
same name. Packages are rarely backed by a resource which could be used
to identify their existence. It's odd that the package that is returned
from Class.getPackage() depends on the order of class loading.
For example, if user code declares a "public class javax.swing.JNotSwing
{}", then the following program:
public class Test {
public static void main(String[] args) {
JNotSwing c1 = new JNotSwing();
Package p1 = c1.getClass().getPackage();
JComponent c2 = new JLabel();
Package p2 = c2.getClass().getPackage();
JComponent c3 = new JPanel();
Package p3 = c3.getClass().getPackage();
System.out.println("p1: " + p1);
System.out.println("p2: " + p2);
System.out.println("p3: " + p3);
}
}
Prints:
p1: package javax.swing
p2: package javax.swing, Java Platform API Specification, version 1.9
p3: package javax.swing, Java Platform API Specification, version 1.9
Now if the order of class loading is changed:
public class Test {
public static void main(String[] args) {
JComponent c2 = new JLabel();
Package p2 = c2.getClass().getPackage();
JNotSwing c1 = new JNotSwing();
Package p1 = c1.getClass().getPackage();
JComponent c3 = new JPanel();
Package p3 = c3.getClass().getPackage();
System.out.println("p1: " + p1);
System.out.println("p2: " + p2);
System.out.println("p3: " + p3);
}
}
The following is printed:
p1: package javax.swing, Java Platform API Specification, version 1.9
p2: package javax.swing, Java Platform API Specification, version 1.9
p3: package javax.swing, Java Platform API Specification, version 1.9
So reliable "sealed" packages can only exist if they are defined
in-advance or at least one class from a particular sealed package is
loaded by the authoritative ClassLoader before a child of that loader is
given a chance to load classes.
A: So perhaps, to support class-loading-order independent behaviour, a
descendant ClassLoader could be given a chance to define it's own
package although the ancestor has already defined one with the same
name, unless this is a sealed package of course.
B: A backwards-incompatible and more restrictive strategy could be to
prevent an ancestor ClassLoader from defining a Package if one of
descendants in the chain leading from initiating ClassLoader to the
ancestor already defines a package with the same name. This would not
prevent loading of such "conflicting" classes for the entire system, but
only for a particular code that happens to be defined by the ClassLoader
that (or it's ancestor) violates the constraint that a descendant
ClassLoader must not define classes in the package of the same name as
ancestor ClassLoader. It would prevent classes loaded by a violating
ClassLoader from accessing classes in sealed packages, which might be
enough to enforce intra-package access restrictions...
How does Jigsaw solve this puzzle?
Regards, Peter
>
> 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
>>>
>>
More information about the core-libs-dev
mailing list