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
