RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Claes Redestad
claes.redestad at oracle.com
Wed Oct 22 12:40:38 UTC 2014
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?
/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
>>>
More information about the core-libs-dev
mailing list