Hello Peter,
Am Fri, 24 Oct 2014 18:27:38 +0200
schrieb Peter Levart :
> > 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 packa
On 10/23/2014 6:26 AM, Claes Redestad wrote:
http://cr.openjdk.java.net/~redestad/8060130/webrev.10
Looks good.
Mandy
On 10/23/2014 03:27 AM, Bernd Eckenfels wrote:
Am Wed, 22 Oct 2014 14:56:59 -0700
schrieb Mandy Chung :
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 th
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
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 t
Am Wed, 22 Oct 2014 14:56:59 -0700
schrieb Mandy Chung :
> 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 th
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 b
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 fil
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 (p
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
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 d
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:
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
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
(
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 syst
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 a
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
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
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 ag
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
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, CachedMan
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
So we all came to same conclusions after all. I would call this a
redundant concurrent "initialization" ;-)
Regards, Peter
On 10/13/2014 05:06 PM, Claes Redestad wrote:
Hi Peter,
On 10/13/2014 04:32 PM, Peter Levart wrote:
On 10/12/2014 04:09 AM, Claes Redestad wrote:
http://cr.openjdk.ja
Hi 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);
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
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.put
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 t
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
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 t
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 ha
Am Fri, 10 Oct 2014 17:31:41 -0700
schrieb Mandy Chung :
> 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
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()):
CachedManife
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);
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
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
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 yo
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
wrote:
> Hi all,
>
> please review this patch which attempts to clean up synchronization and
> improve scalability when
> defining and gettin
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
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 ha
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 t
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 Pa
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
42 matches
Mail list logo
