Good idea.
Mandy
> On May 13, 2016, at 12:10 AM, Peter Levart <peter.lev...@gmail.com> 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:
>
>
> /*
> * @test
> * @bug 8029891
> * @summary Test that the Properties.get() method does not synchronize any more
> * @run main CheckUnsynchronizedGet
> */
> public class CheckUnsynchronizedGet {
>
> public static void main(String[] args) {
> Properties props = new Properties();
> synchronized (props) {
> props.setProperty("key", "value");
> String value =
> CompletableFuture.supplyAsync(() ->
> props.getProperty("key")).join();
> assert "value".equals(value);
> }
> }
> }
>
> What do you think?
>
> Regards, Peter
>
> On 05/13/2016 01:55 AM, Brent Christian wrote:
>> 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 <brent.christ...@oracle.com>
>>>> 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<Object, Object> map =
>>>>> 1153 new ConcurrentHashMap<>(8);
>>>>>
>>>>> to the beginning and add a comment describing what are lock-free and what
>>>>> are synchronized (basically some part of your summary below).
>>>>>
>>>>> Also a comment in Hashtable::defaultWriteHashtable and readHashtable
>>>>> methods to mention that they are overridden by Properties.
>>>>
>>>> Good ideas.
>>>>
>>>>> In the GetResource.java test, what is the reason taking out:
>>>>> 73 URL u2 =
>>>>> Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
>>>>>
>>>>
>>>> It was coming back null and failing the test, I think because
>>>> Jigsaw moved CalendarData into a different module (jdk.localedata, I
>>>> believe?).
>>>
>>> src/java.base/share/classes/sun/util/resources/CalendarData.properties is
>>> still in java.base. This is due to resource encapsulation. Unnamed module
>>> calling ClassLoader::getResource of a resource in a named module returns
>>> null.
>>>
>>>
>>>> I fiddled with @modules a bit, but stopped when I discovered that when
>>>> the test deadlocks, it hangs on the first call to getResource() and
>>>> doesn't get to this line.
>>>>
>>>> I can look into getting it to load CalendarData again, if you like.
>>>
>>> The launcher and builtin class loader implementation has changed so much
>>> that the code path exercised JDK-6977738 no longer exists. I think
>>> dropping line 73 is okay.
>>>
>>> Mandy
>>>
>
