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

[David Holmes]

Hi Claes,

In the test you should use the ProcessTools from the testlibrary not 
mess with System.getProperty("java.home") and create your own processes 
directly. The test should not require a full JDK to run, but should 
execute on a JRE or any compact profile.

Thanks,
David

On 21/10/2014 11:43 PM, Claes Redestad wrote:
> 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
>>