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

Mon, 13 Oct 2014 02:06:13 -0700 

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

Reply via email to