From: core-libs-dev on behalf of Brent
Christian
Sent: Tuesday, May 17, 2016 8:46 PM
To: Mandy Chung; David Holmes
Cc: core-libs-dev
Subject: Re: RFR 8029891 : Deadlock detected in
java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi!
Here's the
Hi Brent,
The unsynchronized test looks good.
Let's hope that we didn't miss any hidden detail. We'll see how this
works out when people get hold of new build containing this change.
Regards, Peter
On 05/18/2016 03:46 AM, Brent Christian wrote:
Hi!
Here's the final(?) webrev:
http://cr.
On 18/05/2016 11:46 AM, Brent Christian wrote:
Hi!
Here's the final(?) webrev:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/
with the following changes since the last one:
No further comments from me.
Thanks,
David
* The @hidden JavaDoc tag has been removed. It can't be u
> On May 17, 2016, at 6:46 PM, Brent Christian
> wrote:
>
> Hi!
>
> Here's the final(?) webrev:
> http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/
+1
Nit: line 132, 134 in PropertiesSerialization.java test are long lines that
should be wrapped to next line.
Mandy
Hi!
Here's the final(?) webrev:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/
with the following changes since the last one:
* The @hidden JavaDoc tag has been removed. It can't be used due to
methods disappearing from the API page. Living with the additional
JavaDoc is the
> On May 12, 2016, at 5:58 PM, David Holmes wrote:
>>
>> With all of the inherited methods @hidden, it looks like that section
>> is left out altogether.
>
> Okay, so I have to say @hidden seems to me to be seriously flawed! If a class
> has a method that can be called then IMHO that has to be
> On May 12, 2016, at 4:55 PM, Brent Christian
> wrote:
>
> Update to the test, and additional comments:
>
> http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/
>
+1
Mandy
Good idea.
Mandy
> On May 13, 2016, at 12:10 AM, Peter Levart wrote:
>
> Hi Brent,
>
>
> This looks good. It might also be a good idea to add a test that checks this
> new implementation detail (the unsynchronized get) that new code will become
> dependent upon, for example:
>
>
> /*
> *
Note that there is a possible initialization circularity introduced by
this patch, which I think is harmless because:
- ThreadLocalRandom has just recently been enhanced to cope with such
situations - CHM needs ThreadLocalRandom in put() for example, when new
entry is being inserted, TLR invok
Hi Brent,
This looks good. It might also be a good idea to add a test that checks
this new implementation detail (the unsynchronized get) that new code
will become dependent upon, for example:
/*
* @test
* @bug 8029891
* @summary Test that the Properties.get() method does not synchronize
Hi Brent,
On 13/05/2016 7:02 AM, Brent Christian wrote:
Hi, David
On 5/11/16 8:39 PM, David Holmes wrote:
On 11/05/2016 7:56 AM, Brent Christian wrote:
While good progress was made during the original code review, all of the
overridden methods in Properties caused an explosion of unnecessary
Update to the test, and additional comments:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/
Thanks,
-Brent
On 5/12/16 4:15 PM, Mandy Chung wrote:
On May 12, 2016, at 2:44 PM, Brent Christian wrote:
On 5/12/16 11:44 AM, Mandy Chung wrote:
This patch looks good.
To help fut
> On May 12, 2016, at 2:44 PM, Brent Christian
> wrote:
>
> On 5/12/16 11:44 AM, Mandy Chung wrote:
>>
>> This patch looks good.
>>
>> To help future readers to understand this, it may be better to move:
>> 1152 private transient ConcurrentHashMap map =
>> 1153 new ConcurrentH
On 5/12/16 11:44 AM, Mandy Chung wrote:
This patch looks good.
To help future readers to understand this, it may be better to move:
1152 private transient ConcurrentHashMap map =
1153 new ConcurrentHashMap<>(8);
to the beginning and add a comment describing what are lock-free a
Hi, David
On 5/11/16 8:39 PM, David Holmes wrote:
On 11/05/2016 7:56 AM, Brent Christian wrote:
While good progress was made during the original code review, all of the
overridden methods in Properties caused an explosion of unnecessary
JavaDoc (see specdiff at [2]). With the recent fix of 807
> On May 11, 2016, at 8:39 PM, David Holmes wrote:
>
>> While good progress was made during the original code review, all of the
>> overridden methods in Properties caused an explosion of unnecessary
>> JavaDoc (see specdiff at [2]). With the recent fix of 8073100 (new
>> "@hidden" JavaDoc tag)
Hi Brent,
>
> A new webrev is here:
> http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/
This patch looks good.
To help future readers to understand this, it may be better to move:
1152 private transient ConcurrentHashMap map =
1153 new ConcurrentHashMap<>(8);
to the beginn
Hi Brent,
On 11/05/2016 7:56 AM, Brent Christian wrote:
Hi! Welcome back to 8029891, "Deadlock detected in
java/lang/ClassLoader/deadlock/GetResource.java". Our previous
discussion is at [1].
Briefly, java.util.Properties isa Hashtable, which synchronizes on
itself for any access. System.getP
On 5/10/16 2:56 PM, Brent Christian wrote:
While good progress was made during the original code review, all of the
overridden methods in Properties caused an explosion of unnecessary
JavaDoc (see specdiff at [2]). With the recent fix of 8073100 (new
"@hidden" JavaDoc tag), we can now avoid the
Hi! Welcome back to 8029891, "Deadlock detected in
java/lang/ClassLoader/deadlock/GetResource.java". Our previous
discussion is at [1].
Briefly, java.util.Properties isa Hashtable, which synchronizes on
itself for any access. System.getProperties() returns the Properties
instance accessed b
> On May 15, 2015, at 12:09 PM, Brent Christian
> wrote:
>
> Updated webrev:
>
> http://cr.openjdk.java.net/~bchristi/8029891/webrev.1/
>
> I have restored synchronization to modification methods, and to my
> interpretation of "bulk read" operations:
>
>* forEach
>* replaceAll
>
Hi, Daniel
You're right, thanks.
The situation is the same for elements(). I've updated these in my
local repo.
I checked through the methods that return a Set/Enumeration/etc, and I
believe there's also an issue with entrySet(). The returned Set should
be backed by the map, and support r
On 16/05/2015 5:09 AM, Brent Christian wrote:
On 5/13/15 8:53 AM, Mandy Chung wrote:
On May 12, 2015, at 2:26 PM, Peter Levart
wrote:
On 05/12/2015 10:49 PM, Mandy Chung wrote:
But I think it should be pretty safe to make the
java.util.Properties object override all Hashtable methods and
deleg
Hi Brent,
1163 @Override
1164 public Enumeration keys() {
1165 return map.keys();
1166 }
I wonder if you should use:
public Enumeration keys() {
return Collections.enumeration(map.keySet());
}
instead.
ConcurrentHashMap.keys() returns an Enumeration which is also an
It
On 5/13/15 8:53 AM, Mandy Chung wrote:
On May 12, 2015, at 2:26 PM, Peter Levart
wrote:
On 05/12/2015 10:49 PM, Mandy Chung wrote:
But I think it should be pretty safe to make the
java.util.Properties object override all Hashtable methods and
delegate to internal CMH so that:
- all modification
On 05/13/2015 01:45 AM, Paul Sandoz wrote:
On May 12, 2015, at 10:49 PM, Mandy Chung wrote:
How likely does existing code do this? A proper way is to call
System.setProperty. One pattern I found on System.setProperties is like this
to add a system property of key/value pair:
Properties p
> On May 12, 2015, at 2:26 PM, Peter Levart wrote:
>
>
>
> On 05/12/2015 10:49 PM, Mandy Chung wrote:
>>> But I think it should be pretty safe to make the java.util.Properties
>>> object override all Hashtable methods and delegate to internal CMH so that:
>>> - all modification methods + all
On May 12, 2015, at 10:49 PM, Mandy Chung wrote:
>>
>> Ah, I understand Mandy now. You are talking about using special Properties
>> implementation just for system properties. Unfortunately, this is currently
>> valid code:
>>
>> Properties props = new Properties();
>> ...
>> System.setPropert
On 05/12/2015 10:49 PM, Mandy Chung wrote:
But I think it should be pretty safe to make the java.util.Properties
object override all Hashtable methods and delegate to internal CMH so
that:
- all modification methods + all bulk read methods such as
(keySet().toArray, values.toArray) are made s
On 05/11/2015 11:41 PM, Peter Levart wrote:
On 05/12/2015 07:41 AM, Peter Levart wrote:
Taking another look at this deadlock issue and the compatibility
concerns, I wonder if we should keep this change as a special
implementation for system properties rather than having this change to
java.u
On 05/12/2015 07:41 AM, Peter Levart wrote:
Taking another look at this deadlock issue and the compatibility
concerns, I wonder if we should keep this change as a special
implementation for system properties rather than having this change to
java.util.Properties class. Properties is a Hashtabl
On 05/12/2015 01:43 AM, Brent Christian wrote:
Hi, Mandy. Thanks for having a look.
On 5/11/15 12:09 PM, Mandy Chung wrote:
~/WORK/8029891/plevartI'd like Daniel's suggestion to have the load*
method to putAll entries in one go after the input reader/stream/xml
file is loaded and parsed rath
> On May 11, 2015, at 4:43 PM, Brent Christian
> wrote:
>
> Hi, Mandy. Thanks for having a look.
>
> On 5/11/15 12:09 PM, Mandy Chung wrote:
>> ~/WORK/8029891/plevartI'd like Daniel's suggestion to have the load*
>> method to putAll entries in one go after the input reader/stream/xml
>> file
Hi, Mandy. Thanks for having a look.
On 5/11/15 12:09 PM, Mandy Chung wrote:
~/WORK/8029891/plevartI'd like Daniel's suggestion to have the load*
method to putAll entries in one go after the input reader/stream/xml
file is loaded and parsed rather than adding one entry at a time.
I also like
Hi Brent,
On 05/04/2015 09:11 AM, Brent Christian wrote:
Hi,
Please review this fix, courtesy of Peter Levart (thanks!), that I
would like to get in.
https://bugs.openjdk.java.net/browse/JDK-8029891
http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/
Thanks for taking this on. I haven'
On 06/05/15 11:41, David Holmes wrote:
I don't think you want to de-synchronize the load* methods - you don't
want two threads calling load concurrently. But that then raises the
problem of concurrent modification while a load is in progress.
Synchronization ensures serialization and by removing
On 6/05/2015 5:57 PM, Peter Levart wrote:
On 05/05/2015 03:25 AM, David Holmes wrote:
Hi Brent,
On 5/05/2015 2:11 AM, Brent Christian wrote:
Hi,
Please review this fix, courtesy of Peter Levart (thanks!), that I would
like to get in.
https://bugs.openjdk.java.net/browse/JDK-8029891
http://
On 05/05/2015 03:25 AM, David Holmes wrote:
Hi Brent,
On 5/05/2015 2:11 AM, Brent Christian wrote:
Hi,
Please review this fix, courtesy of Peter Levart (thanks!), that I would
like to get in.
https://bugs.openjdk.java.net/browse/JDK-8029891
http://cr.openjdk.java.net/~bchristi/8029891/webre
Hi Brent,
On 5/05/2015 2:11 AM, Brent Christian wrote:
Hi,
Please review this fix, courtesy of Peter Levart (thanks!), that I would
like to get in.
https://bugs.openjdk.java.net/browse/JDK-8029891
http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/
There is some discussion of it in the bug
Hi Brent,
I think all you need for test is this:
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.Stream;
public class CheckOverrides {
public static void main(String[] args) {
Set p
On 05/04/2015 06:11 PM, Brent Christian wrote:
Hi,
Please review this fix, courtesy of Peter Levart (thanks!), that I
would like to get in.
https://bugs.openjdk.java.net/browse/JDK-8029891
http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/
There is some discussion of it in the bug repo
Hi,
Please review this fix, courtesy of Peter Levart (thanks!), that I would
like to get in.
https://bugs.openjdk.java.net/browse/JDK-8029891
http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/
There is some discussion of it in the bug report, starting at 2014-12-31.
The problem, as state
42 matches
Mail list logo
