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

[David Holmes]

On 22/10/2014 10:40 PM, Claes Redestad wrote:
> Thanks! Updated:
>
> http://cr.openjdk.java.net/~redestad/8060130/webrev.07/
>
> On a related note,
> java/lang/invoke/lambda/LambdaAccessControlDoPrivilegedTest.java is
> failing when I run make TEST=jdk_lang test from jdk9-dev, and it seems
> to be due to the test expecting java.home to be set to a JRE dir inside
> a JDK. I naïvely copied my first approach from how
> sun/misc/JarIndex/JarIndexMergeForClassLoaderTest.java discovers the jar
> binary. Both these tests seems flawed and should use
> jdk.testlibrary.JDKToolFinder.getJDKTool("jar"), no?

Yes. :) They should just use the available jar tool (normally from the 
"compile JDK").

Thanks,
David

> /Claes
>
> On 10/22/2014 04:23 AM, David Holmes wrote:
>> 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
>>>>
>