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

[Bernd Eckenfels]

Hello,

one thing I wonder - in the old and in the new case there is nothing
in definePackage() guaring the parents from getting to know the new
package and causing a conflict (as the atomic insert is only on the
specific packages table. Are those (parent and system package)
immutable?

Gruss
Bernd

the package  Am Tue, 21 Oct 2014 15:43:07
+0200 schrieb Claes Redestad <claes.redestad at oracle.com>:

> Hi Mandy,
> 
>   thanks for the review!
> 
> On 10/15/2014 03:07 AM, Mandy Chung wrote:
> > Claes, Peter,
> >
> >   Thanks for the revised webrev and Peter's thorough review.
> > webrev.05 looks much better.  My comment is mostly minor.
> >
> >
> > ClassLoader.java
> >
> > line 1582-1586 - I suggest to get rid of the "oldpkg" variable
> > (it's really the package to be used and not an old pkg).
> >
> >     pkg = new Package(name, specTitle, specVersion, specVendor,
> >                             implTitle, implVersion, implVendor,
> >                             sealBase, this);
> >     if (packages.putIfAbsent(name, pkg) != null) {
> >         throw new IllegalArgumentException(name + " already
> > defined"); }
> >     return pkg;
> >
> > line 1634-1635: nit: the pkgName variable is not really needed.
> >    it's in the existing code and probably good to remove it.
> >    Package.java
> >
> > line 473: maybe better to leave the ClassLoader parameter in the 
> > constructor.
> >    I thought about adding a comment saying that this private
> > constructor is only used for system package but keeping the loader
> > parameter makes it more explicit.
> >
> > line 569: nit: formatting - indent to the right to align the first 
> > parameter
> >    to new Package(...)
> 
> Fixed.
> 
> >
> > line 621-623: is this really needed?  Uncontended case seems to be
> >    the common case.  It seems the synchronized overhead would be
> >    insignificant.
> 
> I'd prefer sticking to the double-checked idiom here, unless you
> insist.
> 
> I've cleaned up the code to avoid assignment in if-clauses, which 
> according to
> anonymous sources makes the code more readable. Perhaps this addresses
> some of your concerns?
> 
> >
> > line 624: a space is missing between synchronized and "("
> 
> Fixed.
> 
> >
> > Looks like there is one test 
> > jdk/test/java/lang/ClassLoader/GetPackage.java
> > about packages.  Can you add a new test to verify the system
> > packages as that
> > is one major change in your patch?
> 
> http://cr.openjdk.java.net/~redestad/8060130/webrev.06
> 
> Since I don't want to add binary JAR files, I opted to add a test
> which creates two JAR files,
> each with a single class (with/without manifest) and then proceeds to 
> spawn processes
> to verify that:
> 
> - when the JAR with manifest is on bootclasspath, the package can be 
> found via
> Package.getSystemPackage and the package object reflects values added
> to the manifest
> - when the JAR without manifest is on bootclaspath, the package can
> be found via
> Package.getSystemPackage but is empty apart from name
> - adding the test.classes directory to bootclasspath behaves the same
> as adding the JAR
> without manifest
> - for any case where the class/package is not on the bootclasspath,
> the package information
> can not be found via Package.getSystemPackage().
> 
> Does this cover everything?
> 
> I guess there might be a way to make @run main/othervm or 
> main/bootclasspath pick
> up a dynamically generated JAR file, but I've failed to find a way
> that would make this work without
> pregenerating the JARs. Suggestions on how this can be simplified are 
> welcome.
> 
> /Claes
> 
> >
> > Thanks
> > Mandy
> >