Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/23/2014 03:27 AM, Bernd Eckenfels wrote: Am Wed, 22 Oct 2014 14:56:59 -0700 schrieb Mandy Chung mandy.ch...@oracle.com: I guess your question is related to my comment about two class loaders can define classes in a package of the same name (two different runtime packages). ClassLoader.getPackage(s) assumes there is only one Package for each name in the class loader chain which seems wrong to me. Claes has filed a bug to track this: https://bugs.openjdk.java.net/browse/JDK-8061804 Yes, I think thats the same, Mandy. One option for solving that in a still performant (lockless on hot path) way would be a ConcurrentSet of package names used for the initial decision if the hierachy needs traversed (and why may define a package). With a set of strings it would not keep the packages alive so it can be global. Not sure if it would need a cleanup mechanism.. hmm. Gruss Bernd Hi Bernd, As it is perfectly legal for two unrelated classloaders (for example peers sharing common parent) to each define it's own distinct class with the same name (imagine two J2EE apps each using it's own version of the same library bundled with them), so should two unrelated classloaders be able to define two distinct Packages with same name - and they can do that. Standard delegation model makes sure that a class with a particular name can only be defined by one classloader in the delegation chain from initiating to the bootstrap classloader, but not all classloaders follow this rule (for example WAR classloader in web containers). Packages also wish to follow this rule, but fail, since the existence of a package is established only when 1st class from that package is loaded. Something similar is achieved by making packages sealed, but not all packages are sealed, and I think that even distinct sealed packages with same name can exist in unrelated classloaders. So the most consistent thing to do would be to allow each classloader to define it's own packages (unless ancestors define a sealed package with same name) and to consistently return the Package defined by the Class' defining classloader from the Class.getPackage() method. ClassLoader.getPackages() method should then return a concatenation of defined packages from the invoked classloader up through ancestors to the bootstrap classloader and this list may contain Packages with non-unique names. Regards, Peter
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/23/2014 6:26 AM, Claes Redestad wrote: http://cr.openjdk.java.net/~redestad/8060130/webrev.10 Looks good. Mandy
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Hello Peter, Am Fri, 24 Oct 2014 18:27:38 +0200 schrieb Peter Levart peter.lev...@gmail.com: One option for solving that in a still performant (lockless on hot path) way would be a ConcurrentSet of package names used for the initial decision if the hierachy needs traversed (and why may define a package). With a set of strings it would not keep the packages alive so it can be global. Not sure if it would need a cleanup mechanism.. hmm. As it is perfectly legal for two unrelated classloaders (for example peers sharing common parent) to each define it's own distinct class with the same name (imagine two J2EE apps each using it's own version of the same library bundled with them), so should two unrelated classloaders be able to define two distinct Packages with same name - and they can do that. Yes, the set is only about determine with an atomic operation if you need to traverse the tree or not. So for the case that a package is homed by a single class loader, that class loader will make an entry into the set (and suceed) and then does not need to do more parent traversal. All other class loaders will see there is already the package name reserved, and then they will need to coordinate with their parents. But its just an idea after inspecting the changes, not a real analysis of all usage models. Maybe it is enough to do it for sealed packages. Gruss Bernd Standard delegation model makes sure that a class with a particular name can only be defined by one classloader in the delegation chain from initiating to the bootstrap classloader, but not all classloaders follow this rule (for example WAR classloader in web containers). Packages also wish to follow this rule, but fail, since the existence of a package is established only when 1st class from that package is loaded. Something similar is achieved by making packages sealed, but not all packages are sealed, and I think that even distinct sealed packages with same name can exist in unrelated classloaders. So the most consistent thing to do would be to allow each classloader to define it's own packages (unless ancestors define a sealed package with same name) and to consistently return the Package defined by the Class' defining classloader from the Class.getPackage() method. ClassLoader.getPackages() method should then return a concatenation of defined packages from the invoked classloader up through ancestors to the bootstrap classloader and this list may contain Packages with non-unique names. Regards, Peter
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/23/2014 02:54 AM, Mandy Chung wrote: On 10/22/2014 4:58 PM, Claes Redestad wrote: This test uses a special class loader that delegates to null class loader only. Since you have the verification in place, it'd be good to also call Package.getPackage and Package.getPackages to verify that the same package2 instance is returned. As a sanity check, you could check java.lang package must be present. I've added some sanity checks as suggested. Sadly, it seems that since the application classloader will define and load package2 first in this test, there'll always be a Package defined in the ClassLoader.pkgs that will mask the Package instance in Package.pkgs when retrieved via the public methods in Package. Can you remove package2/Class2.class after you create the jar files? Sorry for the confusion. I realized I did some of my test development on an unpatched JDK and got caught up in a subtle issue. Prior to the changes proposed, calling into Package.getSystemPackages() actually created new Package objects on each call(!!), breaking the assumption that only one Package object will ever be defined. The proposed patch ensures we don't create new objects and that only one Package object can ever be created for a system package. This bug(?) was partially hidden by the fact that calling Package.getPackage() cached Package objects in the ClassLoader package maps, thus subsequent calls to Package.getPackages() would provide identical objects on subsequent calls. I've updated the test to verify Package identity (which it fails to do on an unpatched JDK). If JDK-8061804 is resolved, we could change to check for identity in the Package.getPackages() case, which would improve the specificness of the test... For now, perhaps we could trick things in this test and put our dummy class into java.lang in our test here and ensure that the package retrieved is identical. Worth the hassle? What I meant is to check if the returned Package contains java.lang package that always exists in the system packages. You don't need to put a dummy class in java.lang to get java.lang Package since it must be defined in the running VM. e.g. you can refactor line 169-176 to a findPackage method that returns Package matching the given package name such that in line 164 you can call findPackage(java.lang) that should return non-null. http://cr.openjdk.java.net/~redestad/8060130/webrev.10 /Claes
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
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
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/21/2014 6:43 AM, Claes Redestad wrote: http://cr.openjdk.java.net/~redestad/8060130/webrev.07/ Looks good. A minor nit: Package.java line 636: it can return EMPTY_MANIFEST that will set manifest to non-null so that an invalid file would avoid opening the file every time getManifest is called (although this case rarely happens). line 639 can then be simplified. GetSystemPackage.java test This is great. Thanks for adding the test. Can you break the long lines such as the call to JarBuilder.addClassFile and runSubprocess. Have you run this on windows? I think you need to use File.separator rather than / in the -Xbootclasspath/p argument. This test uses a special class loader that delegates to null class loader only. Since you have the verification in place, it'd be good to also call Package.getPackage and Package.getPackages to verify that the same package2 instance is returned. As a sanity check, you could check java.lang package must be present. In the verifyPackage, it throws Exception. You can simply throw RuntimeException that would avoid the need of the checked exception. Nit: line 35-37, 43-46 - no need to declare these import as java.lang.* are imported by default. Copyright year should be 2014. 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? Looks okay. Initialize manifest to EMPTY_MANIFEST would clean that a little. 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: Thanks for adding the test. - 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? This is great. See my comment above. 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. What you have is fine. The other way is to do it in a shell test that is not preferable as you would have to manage FS for different OSes etc. Mandy
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/21/2014 2:08 PM, Bernd Eckenfels wrote: 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? Each class loader maintains its own package map. I guess your question is related to my comment about two class loaders can define classes in a package of the same name (two different runtime packages). ClassLoader.getPackage(s) assumes there is only one Package for each name in the class loader chain which seems wrong to me. Claes has filed a bug to track this: https://bugs.openjdk.java.net/browse/JDK-8061804 Mandy [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-October/029146.html
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 2014-10-22 23:35, Mandy Chung wrote: On 10/21/2014 6:43 AM, Claes Redestad wrote: http://cr.openjdk.java.net/~redestad/8060130/webrev.07/ Looks good. Thanks! A minor nit: Package.java line 636: it can return EMPTY_MANIFEST that will set manifest to non-null so that an invalid file would avoid opening the file every time getManifest is called (although this case rarely happens). line 639 can then be simplified. We need to consider the case where JarInputStream.getManifest() returns null, which would mean we'd either end up no simpler on line 639 or simply adding a similar check around the value returned from jis.getManifest(). I also think I saw that referencing the static EMPTY_PACKAGES from within the privileged anonymous class adds synthetic access methods... More importantly, the current approach should already ensure any file is only scanned once by virtue of always setting manifest to a non-null value in the end. No? GetSystemPackage.java test This is great. Thanks for adding the test. Thanks! Can you break the long lines such as the call to JarBuilder.addClassFile and runSubprocess. Sure. Cleaned it up a bit. Have you run this on windows? I think you need to use File.separator rather than / in the -Xbootclasspath/p argument. Yes, I've run the test via JPRT and verified it gets picked up and run using the default testset, so Windows, Solaris, Mac and Linux should be covered. This test uses a special class loader that delegates to null class loader only. Since you have the verification in place, it'd be good to also call Package.getPackage and Package.getPackages to verify that the same package2 instance is returned. As a sanity check, you could check java.lang package must be present. I've added some sanity checks as suggested. Sadly, it seems that since the application classloader will define and load package2 first in this test, there'll always be a Package defined in the ClassLoader.pkgs that will mask the Package instance in Package.pkgs when retrieved via the public methods in Package. If JDK-8061804 is resolved, we could change to check for identity in the Package.getPackages() case, which would improve the specificness of the test... For now, perhaps we could trick things in this test and put our dummy class into java.lang in our test here and ensure that the package retrieved is identical. Worth the hassle? In the verifyPackage, it throws Exception. You can simply throw RuntimeException that would avoid the need of the checked exception. Sure. I've restructured a bit to keep line lengths in check. Nit: line 35-37, 43-46 - no need to declare these import as java.lang.* are imported by default. Some IDEs... Copyright year should be 2014. Fixed. http://cr.openjdk.java.net/~redestad/8060130/webrev.08 /Claes 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? Looks okay. Initialize manifest to EMPTY_MANIFEST would clean that a little. 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: Thanks for adding the test. - 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? This is great. See my comment above. 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. What you have is fine. The other way is to do it in a shell test that is not preferable as you would have to manage FS for different OSes etc. Mandy
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/22/2014 4:58 PM, Claes Redestad wrote: A minor nit: Package.java line 636: it can return EMPTY_MANIFEST that will set manifest to non-null so that an invalid file would avoid opening the file every time getManifest is called (although this case rarely happens). line 639 can then be simplified. We need to consider the case where JarInputStream.getManifest() returns null, which would mean we'd either end up no simpler on line 639 or simply adding a similar check around the value returned from jis.getManifest(). I also think I saw that referencing the static EMPTY_PACKAGES from within the privileged anonymous class adds synthetic access methods... More importantly, the current approach should already ensure any file is only scanned once by virtue of always setting manifest to a non-null value in the end. No? That's right. I missed that you did set manifest to non-null in line 639. That's fine then. Yes, I've run the test via JPRT and verified it gets picked up and run using the default testset, so Windows, Solaris, Mac and Linux should be covered. That's good. This test uses a special class loader that delegates to null class loader only. Since you have the verification in place, it'd be good to also call Package.getPackage and Package.getPackages to verify that the same package2 instance is returned. As a sanity check, you could check java.lang package must be present. I've added some sanity checks as suggested. Sadly, it seems that since the application classloader will define and load package2 first in this test, there'll always be a Package defined in the ClassLoader.pkgs that will mask the Package instance in Package.pkgs when retrieved via the public methods in Package. Can you remove package2/Class2.class after you create the jar files? If JDK-8061804 is resolved, we could change to check for identity in the Package.getPackages() case, which would improve the specificness of the test... For now, perhaps we could trick things in this test and put our dummy class into java.lang in our test here and ensure that the package retrieved is identical. Worth the hassle? What I meant is to check if the returned Package contains java.lang package that always exists in the system packages. You don't need to put a dummy class in java.lang to get java.lang Package since it must be defined in the running VM. e.g. you can refactor line 169-176 to a findPackage method that returns Package matching the given package name such that in line 164 you can call findPackage(java.lang) that should return non-null. Nit: line 35-37, 43-46 - no need to declare these import as java.lang.* are imported by default. Some IDEs... Which IDE are you using? I think it might be your configuration. Thanks Mandy
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Am Wed, 22 Oct 2014 14:56:59 -0700 schrieb Mandy Chung mandy.ch...@oracle.com: I guess your question is related to my comment about two class loaders can define classes in a package of the same name (two different runtime packages). ClassLoader.getPackage(s) assumes there is only one Package for each name in the class loader chain which seems wrong to me. Claes has filed a bug to track this: https://bugs.openjdk.java.net/browse/JDK-8061804 Yes, I think thats the same, Mandy. One option for solving that in a still performant (lockless on hot path) way would be a ConcurrentSet of package names used for the initial decision if the hierachy needs traversed (and why may define a package). With a set of strings it would not keep the packages alive so it can be global. Not sure if it would need a cleanup mechanism.. hmm. Gruss Bernd
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
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
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
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
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
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.redes...@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
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/14/2014 07:22 PM, Mandy Chung wrote: On 10/13/2014 5:50 AM, David M. Lloyd wrote: On 10/10/2014 07:31 PM, 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. I have a little more information on this subject. We've a possible (and somewhat likely) deadlock which occurs because one thread can attempt to define a system class while holding the java.lang.Package#pkgs lock, while another thread can attempt to get a package while defining a system class (while holding the class loader lock). I do not recall whether parallel class loading alleviates this issue. We solved the problem by loading Packages.getPackages() in early (single-threaded) bootstrap. Do you recall what JDK version you observed this possible deadlock? I wonder if the fix for 7001933 [1] in JDK 7 and 6u25 resolved the deadlock problem you ran into. [1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/4a7da412db38 The change would have been observed in early 2011 so I think it was probably a JDK 6 version before 25 (which I did not install until April of that year) - looks like most likely candidate based on my JDK directory is 1.6.0_22. So from my perspective, just getting rid of the synchronization on that field alone makes this change worthwhile. Yes that's what I think too. Mandy -- - DML
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/15/2014 5:43 AM, David M. Lloyd wrote: On 10/14/2014 07:22 PM, Mandy Chung wrote: On 10/13/2014 5:50 AM, David M. Lloyd wrote: I have a little more information on this subject. We've a possible (and somewhat likely) deadlock which occurs because one thread can attempt to define a system class while holding the java.lang.Package#pkgs lock, while another thread can attempt to get a package while defining a system class (while holding the class loader lock). I do not recall whether parallel class loading alleviates this issue. We solved the problem by loading Packages.getPackages() in early (single-threaded) bootstrap. Do you recall what JDK version you observed this possible deadlock? I wonder if the fix for 7001933 [1] in JDK 7 and 6u25 resolved the deadlock problem you ran into. [1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/4a7da412db38 The change would have been observed in early 2011 so I think it was probably a JDK 6 version before 25 (which I did not install until April of that year) - looks like most likely candidate based on my JDK directory is 1.6.0_22. Thanks. It's possible that it's the same deadlock issue. Mandy
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/13/2014 5:50 AM, David M. Lloyd wrote: On 10/10/2014 07:31 PM, 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. I have a little more information on this subject. We've a possible (and somewhat likely) deadlock which occurs because one thread can attempt to define a system class while holding the java.lang.Package#pkgs lock, while another thread can attempt to get a package while defining a system class (while holding the class loader lock). I do not recall whether parallel class loading alleviates this issue. We solved the problem by loading Packages.getPackages() in early (single-threaded) bootstrap. Do you recall what JDK version you observed this possible deadlock? I wonder if the fix for 7001933 [1] in JDK 7 and 6u25 resolved the deadlock problem you ran into. [1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/4a7da412db38 So from my perspective, just getting rid of the synchronization on that field alone makes this change worthwhile. Yes that's what I think too. Mandy
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Claes, Peter,
Thanks for the revised webrev and Peter's thorough review. webrev.05
looks much better. My comment is mostly minor.
On 10/13/2014 8:41 AM, Claes Redestad wrote:
http://cr.openjdk.java.net/~redestad/8060130/webrev.05
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(...)
line 621-623: is this really needed? Uncontended case seems to be
the common case. It seems the synchronized overhead would be
insignificant.
line 624: a space is missing between synchronized and (
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?
Thanks
Mandy
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/13/2014 2:04 AM, Peter Levart wrote: On 10/13/2014 04:18 AM, David Holmes wrote: 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. I don't know the history of the getPackage(s) methods. Since two different class loaders can define a package of the same name and effectively two separate runtime packages, it seems to me that ClassLoader.getPackages should return all Package objects including the duplicated names. Claes - do you mind filing a bug for it? ... 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: ... 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? java.lang.Package is not used at runtime. I think ClassLoader.getPackage(s) method should be revisited whether it should lookup its parent loader. I think your question concerns the runtime package that is not effect by java.lang.Package. Mandy
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
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
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/10/2014 07:31 PM, 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. I have a little more information on this subject. We've a possible (and somewhat likely) deadlock which occurs because one thread can attempt to define a system class while holding the java.lang.Package#pkgs lock, while another thread can attempt to get a package while defining a system class (while holding the class loader lock). I do not recall whether parallel class loading alleviates this issue. We solved the problem by loading Packages.getPackages() in early (single-threaded) bootstrap. So from my perspective, just getting rid of the synchronization on that field alone makes this change worthwhile. -- - DML
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
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.
Right, this would be a throughput-footprint tradeoff: adding more CHM
entries versus taking a small penalty for having to potentially look
things up in all the ancestral classloaders + Packages.pkgs.
I believe the caching was put in place mostly to avoid a chain of
relatively expensive synchronization locks with potential for deadlocks,
not to solve an actual throughput problem in uncontended cases. There
might be a break-off somewhere due to chaining classloaders, but I've
not been able to construct a benchmark where this is the case. I could
swing either way on this, but with a trivial lookup cost even in the
worst case I generally think we should favor footprint here and defer
caching-for-performance to the application if ever necessary.
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.
Very interesting, but since we're not changing behavior and the issue of
classloading order Peter points out can't be resolved by adding
atomicity guarantees, I feel this is out of scope for this cleanup.
Should we file a new bug to examine this?
/Claes
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:
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Hi Claes,
Regarding CachedManifest I have one more comment. It's usually better to
use null/zero as uninitialized state for lazy-initialized fields. You
spare one (volatile) write and don't worry about unsafe publication
which might see the Java-default value instead of the one assigned in
initializer as uninitialized. So summing-up all comments from previous
mails I would write CachedManifest class as:
private static class CachedManifest {
static final Manifest NO_MANIFEST = new Manifest();
final String fileName;
private final URL url;
private volatile Manifest manifest;
CachedManifest(final String fileName) {
this.fileName = fileName;
this.url = AccessController.doPrivileged(new
PrivilegedActionURL() {
public URL run() {
final File file = new File(fileName);
if (file.isFile()) {
try {
return ParseUtil.fileToEncodedURL(file);
} catch (MalformedURLException e) {
}
}
return null;
}
});
}
public URL getURL() {
return url;
}
public Manifest getManifest() {
if (url == null) return null;
Manifest m = manifest;
if (m == null) {
synchronized (this) {
if ((m = manifest) == null) {
manifest = m =
AccessController.doPrivileged(new PrivilegedActionManifest() {
public Manifest run() {
try (FileInputStream fis = new
FileInputStream(fileName);
JarInputStream jis = new
JarInputStream(fis, false))
{
return jis.getManifest();
} catch (IOException e) {
return NO_MANIFEST;
}
}
});
}
}
}
return (m == NO_MANIFEST) ? null : m;
}
}
What do you say?
Regards, Peter
On 10/13/2014 04:32 PM, Peter Levart wrote:
On 10/12/2014 04:09 AM, Claes Redestad wrote:
http://cr.openjdk.java.net/~redestad/8060130/webrev.03/
Hi Claes,
First, one more nit:
1635 if (!map.containsKey(pkgName)) {
1636 map.put(pkgName, pkg);
1637 }
..could be written as:
map.putIfAbsent(pkgName, pkg);
And second, I re-read the code of CachedManifest and noticed the
following. You initialize the volatile instance field 'manifest' like
that:
609 private volatile Manifest manifest = EMPTY_MANIFEST;
So in following code:
631 public Manifest getManifest() {
632 resolveManifest();
633 return manifest;
634 }
635
636 private void resolveManifest() {
637 if (manifest != null || url == null) {
638 return;
639 }
640 synchronized(this) {
641 if (manifest == null) {
642 manifest = AccessController.doPrivileged(new
PrivilegedActionManifest() {
643 public Manifest run() {
644 return loadManifest(fileName);
645 }
646 });
647 }
648 }
649 }
... loadManifest() will never be executed. getManifest() will always
return EMPTY_MANIFEST.
You probably wanted line 637 to be written as:
if (manivest != EMPTY_MANIFEST || url == null) { ...
Likewise in loadManifest(), wou write:
586 private static Manifest loadManifest(String fn) {
587 try (FileInputStream fis = new FileInputStream(fn);
588 JarInputStream jis = new JarInputStream(fis, false))
589 {
590 return jis.getManifest();
591 } catch (IOException e) {
592 return EMPTY_MANIFEST;
593 }
594 }
..but you probably wanted the line 592 to return 'null' instead.
Regards, Peter
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/13/2014 05:15 PM, Peter Levart wrote: Yes, I realized I got this messed up and have prepared a new patch which passes tests and includes your previous comments: http://cr.openjdk.java.net/~redestad/8060130/webrev.04 Hi Claes, Hm, CachedManifest.getManifest() in 4th webrev is still a little faulty. It sometimes returns EMPTY_MANIFEST and sometimes null. I think it should always return null in case when no manifest is found (at least that was the old code behaviour). Regards, Peter
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/13/2014 05:29 PM, Peter Levart wrote: On 10/13/2014 05:15 PM, Peter Levart wrote: Yes, I realized I got this messed up and have prepared a new patch which passes tests and includes your previous comments: http://cr.openjdk.java.net/~redestad/8060130/webrev.04 Hi Claes, Hm, CachedManifest.getManifest() in 4th webrev is still a little faulty. It sometimes returns EMPTY_MANIFEST and sometimes null. I think it should always return null in case when no manifest is found (at least that was the old code behaviour). Hmm, yes, that last little detail of making sure we also set m before returning: -manifest = (m == null ? EMPTY_MANIFEST : m); +manifest = m = (m == null ? EMPTY_MANIFEST : m); http://cr.openjdk.java.net/~redestad/8060130/webrev.05 We rely on returning a non-null manifest from CachedManifest.getManifest() in the current code, so returning null would be a bad idea unless we revert the simplifications to defineSystemPackage. /Claes Regards, Peter
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/13/2014 05:41 PM, Claes Redestad wrote: On 10/13/2014 05:29 PM, Peter Levart wrote: On 10/13/2014 05:15 PM, Peter Levart wrote: Yes, I realized I got this messed up and have prepared a new patch which passes tests and includes your previous comments: http://cr.openjdk.java.net/~redestad/8060130/webrev.04 Hi Claes, Hm, CachedManifest.getManifest() in 4th webrev is still a little faulty. It sometimes returns EMPTY_MANIFEST and sometimes null. I think it should always return null in case when no manifest is found (at least that was the old code behaviour). Hmm, yes, that last little detail of making sure we also set m before returning: -manifest = (m == null ? EMPTY_MANIFEST : m); +manifest = m = (m == null ? EMPTY_MANIFEST : m); http://cr.openjdk.java.net/~redestad/8060130/webrev.05 We rely on returning a non-null manifest from CachedManifest.getManifest() in the current code, so returning null would be a bad idea unless we revert the simplifications to defineSystemPackage. Right, I missed that part. Peter /Claes Regards, Peter
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/12/2014 04:09 AM, Claes Redestad wrote:
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/
Hi Claes,
Just some nits...
- I think you're still using Map as the type of 'pkgs' and 'manifests'
static fields in Package although they are CHMs at runtime.
- To avoid unnecessary (multiple) reads from a volatile field, the
following code in CachedManifest:
631 public Manifest getManifest() {
632 resolveManifest();
633 return manifest;
634 }
635
636 private void resolveManifest() {
637 if (manifest != null || url == null) {
638 return;
639 }
640 synchronized(this) {
641 if (manifest == null) {
642 manifest = AccessController.doPrivileged(new
PrivilegedActionManifest() {
643 public Manifest run() {
644 return loadManifest(fileName);
645 }
646 });
647 }
648 }
649 }
could be written as:
public Manifest getManifest() {
if (url == null) return null;
Manifest m = manifest;
if (m != null) return m;
synchronized (this) {
if ((m = manifest) != null) return m;
manifest = m = AccessControler.doPrivileged();
return m;
}
}
- It would be better to move loadManifest() into the CachedManifest
class since it's only used there as a helper method (to avoid creating
access bridges or having to declare it package-private instead) - well
no, this doesn't help, since it's called from an anonymous inner
PrivilegedAction subclass. What about just inlining it into the
PrivilegeAction's run() method?
I also wonder if the lazy loading of Manifest in CachedManifest is
necessary. If you look at the only usage of CachedManifest.getManifest()
method (in Package.defineSystemPackage()):
CachedManifest cachedManifest = createCachedManifest(fn);
pkgs.putIfAbsent(name, new Package(name,
cachedManifest.getManifest(),
cachedManifest.getURL()));
...you can see that getManifest() is called immediately after obtaining
the CacheManifest. So there's no need for lazy loading. Loading the
Manifest in CachedManifest constructor would be just fine.
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?
Or, call the prototype a 'CachedManifest' and Package just delegate
methods to it. Well...
A noble goal, but this space optimization would only pertain to system
packages if changed only in ClassLoader/Package. You would still have to
have fat Packages because other ClassLoaders define their Packages
with the other Package constructor that doesn't take the Manifest file
name (which would be used as a key to obtain a prototype), but use their
own Manifest parsing (see URLClassLoader.definePackage()) ... There are
just a couple of system packages. So you would have to also change the
API between individual ClassLoader(s) and ClassLoader/Package (see
ClassLoader.definePackage()) to optimize other non-system packages which
outnumber system packages. There are not so many packages after all
(compared to Class(es)).
Regards, Peter
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/12/2014 12:40 PM, Peter Levart wrote: I also wonder if the lazy loading of Manifest in CachedManifest is necessary. If you look at the only usage of CachedManifest.getManifest() method (in Package.defineSystemPackage()): CachedManifest cachedManifest = createCachedManifest(fn); pkgs.putIfAbsent(name, new Package(name, cachedManifest.getManifest(), cachedManifest.getURL())); ...you can see that getManifest() is called immediately after obtaining the CacheManifest. So there's no need for lazy loading. Loading the Manifest in CachedManifest constructor would be just fine. ..Ah, you can do that, yes, but then you have to use synchronization in createCachedManifest in order to avoid redundant concurrent loading of Manifest. And that would be a good thing since then you avoid concurrent resolving of fileName - URL too in one go. Peter
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/12/2014 12:52 PM, Peter Levart wrote: On 10/12/2014 12:40 PM, Peter Levart wrote: I also wonder if the lazy loading of Manifest in CachedManifest is necessary. If you look at the only usage of CachedManifest.getManifest() method (in Package.defineSystemPackage()): CachedManifest cachedManifest = createCachedManifest(fn); pkgs.putIfAbsent(name, new Package(name, cachedManifest.getManifest(), cachedManifest.getURL())); ...you can see that getManifest() is called immediately after obtaining the CacheManifest. So there's no need for lazy loading. Loading the Manifest in CachedManifest constructor would be just fine. ..Ah, you can do that, yes, but then you have to use synchronization in createCachedManifest in order to avoid redundant concurrent loading of Manifest. And that would be a good thing since then you avoid concurrent resolving of fileName - URL too in one go. Peter Scrap that. I see now what you wanted to achieve. You wanted to avoid synchronization on a single monitor when loading manifests while avoiding multiple concurrent loading of same manifest. Lazy loading is fine than. Regards, Peter
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Am Fri, 10 Oct 2014 17:31:41 -0700 schrieb Mandy Chung mandy.ch...@oracle.com: 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. Hmm.. isnt the package logic part of the ClassLoader (defineClass) especially around sealing and security manager? So it would be at least performance critical for startup time? Bernd
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
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.
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
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Hi Mandy, On 10/11/2014 02:31 AM, Mandy Chung wrote: [...] webrev: http://cr.openjdk.java.net/~redestad/8060130/webrev.02/ ClassLoader.java line 272: can you change the declared type as Map. I think it's a bad idea, the class doesn't work if you replace the ConcurrentHashMap by any Map so declaring the field 'packages' as a Map seems wrong to me. At least, it should be a ConcurrentMap, but even with a ConcurrentMap, I think that the guarantee that in definePackage the call to putIfAbstent is amortized O(1) and not something like O(ln n) is important. Note that using interfaces instead of implementations is important in the signature of public or protected method, for a local variable or a private field, which are hidden, there is no real reason to use an interface. I understand that pedagogically having only one rule is appealing but from my own experience, this rule hide one of the corner stone of the OOP, encapsulation, so IMO the rule 'use an interface everywhere you can' does more harm than good because it offers a discorded version of the OO world. cheers, Rémi
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
The commented annotation in ClassLoader.java must be removed (line 268)
// @GuardedBy(itself)
Stanimir
On Fri, Oct 10, 2014 at 6:10 PM, Claes Redestad claes.redes...@oracle.com
wrote:
Hi all,
please review this patch which attempts to clean up synchronization and
improve scalability when
defining and getting java.lang.Package objects.
webrev: http://cr.openjdk.java.net/~redestad/8060130/webrev.02/
bug: https://bugs.openjdk.java.net/browse/JDK-8060130
Testing: jtreg, UTE vm.parallel_class_loading.testlist, various benchmarks
Torturing the retrieval code with a simple microbenchmark[1] shows that
the existing code
could cause bottlenecks, but also that the proposed patch works slightly
faster even in
uncontended cases:
Benchmark Mode Samples Score Score error
Units
baseline, 1 thread:
o.s.SimpleBench.getClassPackagethrpt 10 11.6210.618
ops/us
o.s.SimpleBench.getPackage thrpt 10 41.7543.381
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.0090.000
ops/us
baseline, 2 threads:
o.s.SimpleBench.getClassPackagethrpt 10 7.8841.977
ops/us
o.s.SimpleBench.getPackage thrpt 10 17.0138.079
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.0040.001
ops/us
patch applied, 1 thread:
o.s.SimpleBench.getClassPackagethrpt 10 13.5190.170
ops/us
o.s.SimpleBench.getPackage thrpt 10 59.9990.988
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.0190.001
ops/us
patch applied, 2 threads:
o.s.SimpleBench.getClassPackagethrpt 10 19.1813.688
ops/us
o.s.SimpleBench.getPackage thrpt 10 79.708 31.220
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.0250.006
ops/us
/Claes
[1]
package org.sample;
import org.openjdk.jmh.annotations.*;
@State(Scope.Thread)
public class SimpleBench {
@Benchmark
public Package[] getPackages() {
return Package.getPackages();
}
@Benchmark
public Package getClassPackage() {
return this.getClass().getPackage();
}
@Benchmark
public Package getPackage() {
return Package.getPackage(java.util.zip);
}
}
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
On 10/11/2014 8:17 AM, Remi Forax wrote: Hi Mandy, On 10/11/2014 02:31 AM, Mandy Chung wrote: [...] webrev: http://cr.openjdk.java.net/~redestad/8060130/webrev.02/ ClassLoader.java line 272: can you change the declared type as Map. I think it's a bad idea, the class doesn't work if you replace the ConcurrentHashMap by any Map so declaring the field 'packages' as a Map seems wrong to me. At least, it should be a ConcurrentMap, but even with a ConcurrentMap, I think that the guarantee that in definePackage the call to putIfAbstent is amortized O(1) and not something like O(ln n) is important. Note that using interfaces instead of implementations is important in the signature of public or protected method, for a local variable or a private field, which are hidden, there is no real reason to use an interface. I understand that pedagogically having only one rule is appealing but from my own experience, this rule hide one of the corner stone of the OOP, encapsulation, so IMO the rule 'use an interface everywhere you can' does more harm than good because it offers a discorded version of the OO world. Remi - thanks for catching this. After I sent the comment, I was pondering my suggestion (this specific one) was a bad idea. Declaring it as Map loses the significance that the implementation depends on the packages map being concurrently accessed. Agree. Mandy
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
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
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Hi Claes,
On 11/10/2014 1: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.
Is this a real problem or a theoretical one? I've not previously heard
of getting Package objects as being critical. ConcurrentHashMap improves
the contended case but if we aren't normally contended then it will
degrade performance. How does this perform on real code?
I'm also wondering how this impacts the initialization order of the VM
as we need a lot more classes to be loaded and initialized when the
first Package is created.
Thanks,
David
webrev: http://cr.openjdk.java.net/~redestad/8060130/webrev.02/
bug: https://bugs.openjdk.java.net/browse/JDK-8060130
Testing: jtreg, UTE vm.parallel_class_loading.testlist, various benchmarks
Torturing the retrieval code with a simple microbenchmark[1] shows that
the existing code
could cause bottlenecks, but also that the proposed patch works slightly
faster even in
uncontended cases:
Benchmark Mode Samples Score Score error Units
baseline, 1 thread:
o.s.SimpleBench.getClassPackagethrpt 10 11.6210.618
ops/us
o.s.SimpleBench.getPackage thrpt 10 41.7543.381
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.0090.000
ops/us
baseline, 2 threads:
o.s.SimpleBench.getClassPackagethrpt 10 7.8841.977
ops/us
o.s.SimpleBench.getPackage thrpt 10 17.0138.079
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.0040.001
ops/us
patch applied, 1 thread:
o.s.SimpleBench.getClassPackagethrpt 10 13.5190.170
ops/us
o.s.SimpleBench.getPackage thrpt 10 59.9990.988
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.0190.001
ops/us
patch applied, 2 threads:
o.s.SimpleBench.getClassPackagethrpt 10 19.1813.688
ops/us
o.s.SimpleBench.getPackage thrpt 10 79.708 31.220
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.0250.006
ops/us
/Claes
[1]
package org.sample;
import org.openjdk.jmh.annotations.*;
@State(Scope.Thread)
public class SimpleBench {
@Benchmark
public Package[] getPackages() {
return Package.getPackages();
}
@Benchmark
public Package getClassPackage() {
return this.getClass().getPackage();
}
@Benchmark
public Package getPackage() {
return Package.getPackage(java.util.zip);
}
}
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
Hi David!
On 10/10/2014 05:22 PM, David Holmes wrote:
Hi Claes,
On 11/10/2014 1: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.
Is this a real problem or a theoretical one?
Deadlocks in getPackage() have historically caused issues
(https://bugs.openjdk.java.net/browse/JDK-7001933), so contention can
happen. Still somewhat theoretical, though.
I've not previously heard of getting Package objects as being
critical. ConcurrentHashMap improves the contended case but if we
aren't normally contended then it will degrade performance. How does
this perform on real code?
Experiments (like the trivial microbenchmark below) indicate this
improves performance even in uncontended cases, which can probably be
explained by the removal of explicit synchronization in favor of
volatile reads inside CHM. I've run a number of larger benchmarks mostly
to ensure there are no unexpected regressions, but not found a case
where this patch has a significant impact. There's a small footprint win
in effect by merging two maps in java.lang.Package and not saving
ancestral entries in the ClassLoader packages map which shows up on some
footprint measures and might have more impact when scaling up to larger
apps.
I'm also wondering how this impacts the initialization order of the VM
as we need a lot more classes to be loaded and initialized when the
first Package is created.
Since this is a concern, I've ran a number of internal startup
benchmarks but not found the changes to have any statistical significant
startup impact in either direction. Since ConcurrentHashMap is already
in use by any parallel capable java.lang.ClassLoader, java.lang.Thread
etc, it would seem it'll already be loaded and initialized early.
/Claes
Thanks,
David
webrev: http://cr.openjdk.java.net/~redestad/8060130/webrev.02/
bug: https://bugs.openjdk.java.net/browse/JDK-8060130
Testing: jtreg, UTE vm.parallel_class_loading.testlist, various
benchmarks
Torturing the retrieval code with a simple microbenchmark[1] shows that
the existing code
could cause bottlenecks, but also that the proposed patch works slightly
faster even in
uncontended cases:
Benchmark Mode Samples Score Score error
Units
baseline, 1 thread:
o.s.SimpleBench.getClassPackagethrpt 10 11.621 0.618
ops/us
o.s.SimpleBench.getPackage thrpt 10 41.754 3.381
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.009 0.000
ops/us
baseline, 2 threads:
o.s.SimpleBench.getClassPackagethrpt 10 7.884 1.977
ops/us
o.s.SimpleBench.getPackage thrpt 10 17.013 8.079
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.004 0.001
ops/us
patch applied, 1 thread:
o.s.SimpleBench.getClassPackagethrpt 10 13.519 0.170
ops/us
o.s.SimpleBench.getPackage thrpt 10 59.999 0.988
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.019 0.001
ops/us
patch applied, 2 threads:
o.s.SimpleBench.getClassPackagethrpt 10 19.181 3.688
ops/us
o.s.SimpleBench.getPackage thrpt 10 79.708 31.220
ops/us
o.s.SimpleBench.getPackagesthrpt 10 0.025 0.006
ops/us
/Claes
[1]
package org.sample;
import org.openjdk.jmh.annotations.*;
@State(Scope.Thread)
public class SimpleBench {
@Benchmark
public Package[] getPackages() {
return Package.getPackages();
}
@Benchmark
public Package getClassPackage() {
return this.getClass().getPackage();
}
@Benchmark
public Package getPackage() {
return Package.getPackage(java.util.zip);
}
}
Re: RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package
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.
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.
move line 1623 to 1630 so that the declaration of map is closer to
the assignment.
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.
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 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);
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.
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)?
Mandy
