Re: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)
Hi, I updated the webrev with first two suggestions from Bernd (expireTime instead of createTime and cacheNanos + only use putIfAbsent instead of get followed by putIfAbsent): http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.02/ Thanks, Bernd. The id field in CachedAddresses is necessary for compareTo to never return 0 for two different instances (used as element in ConcurrentSkipListSet). For last two suggestions I'm not sure whether they are desired, so I'm currently leaving them as is. Regards, Peter On 07/01/2014 10:06 PM, Bernd Eckenfels wrote: Looks good, like it, Peter. some nits: instead of adding createTime and cacheNanos, only have a expireAfter? L782: is it better to use putIfAbsent unconditionally, instead of get/putIfAbsent in NameServicdeAddr? L732: I am unsure about the id field, isnt it enough to have the identity equality check for the replacement check and otherwise depend on equals()? L1223: What about moving the cache exiry inside the if (useCache) BTW1: might be the wrong RFR, but considering your good performance numbers for an active cache, would having 100ms or similiar default negative cache time make sense without impacting (visible) the semantic. Gruss Bernd Am Tue, 01 Jul 2014 20:35:57 +0200 schrieb Peter Levart : Hi, I propose a patch for this issue: https://bugs.openjdk.java.net/browse/JDK-7186258 The motivation to re-design caching of InetAddress-es was not this issue though, but a desire to attack synchronization bottlenecks in methods like URL.equals and URL.hashCode which use host name to IP address mapping. I plan to tackle the synchronization in URL in a follow-up proposal, but I wanted to 1st iron-out the "leaves" of the call-tree. Here's the proposed patch: http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.01/ sun.net.InetAddressCachePolicy: - two static methods (get() and getNegative()) were synchronized. Removed synchronization and made underlying fields volatile. - also added a normalization of negative policy in setNegativeIfNotSet(). The logic in InetAddress doesn't cope with negative values distinct from InetAddressCachePolicy.FOREVER (-1), so this was a straight bug. The setIfNotSet() doesn't need this normalization, because checkValue() throws exception if passed-in value < InetAddressCachePolicy.FOREVER. java.net.InetAddress: - complete redesign of caching. Instead of distinct Positive/Negative caches, there's only one cache - a ConcurrentHashMap. The value in the map knows if it contains positive or negative answer. - the design of this cache is similar but much simpler than java.lang.reflect.WeakCache, since it doesn't have to deal with WeakReferences and keys are simpler (just strings - hostnames). Similarity is in how concurrent requests for the same key (hostname) are synchronized when the entry is not cached yet, but still avoid synchronization when entry is cached. This preserves the behaviour of original InetAddress caching code but simplifies it greatly (100+ lines removed). - I tried to preserve the interaction between InetAddress.getLocalHost() and InetAddress.getByName(). The getLocalHost() caches the local host address for 5 seconds privately. When it expires it performs new name service look-up and "refreshes" the entry in the InetAddress.getByName() cache although it has not expired yet. I think this is meant to prevent surprises when getLocalHost() returns newer address than getByName() which is called after that. - I also fixed the JDK-7186258 as a by-product (but don't know yet how to write a test for this issue - any ideas?) I created a JMH benchmark that tests the following methods: - InetAddress.getLocalHost() - InetAddress.getByName() (with positive and negative answer) Here're the results of running on my 4-core (8-threads) i7/Linux: http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/InetAddress.Cache_bench_results.01.pdf The getByNameNegative() test does not show much improvement in patched vs. original code. That's because by default the policy is to NOT cache negative answers. Requests for same hostname to the NameService(s) are synchronized. If "networkaddress.cache.negative.ttl" system property is set to some positive value, results are similar to those of getByNamePositive() test (the default policy for positive caching is 30 seconds). I ran the jtreg tests in test/java/net and have the same score as with original unpatched code. I have 3 failing tests from original and patched runs: JT Harness : Tests that failed java/net/MulticastSocket/Promiscuous.java: Test for interference when two sockets are bound to the same port but joined to different multicast groups java/net/MulticastSocket/SetLoopbackMode.java: Test MulticastSocket.setLoopbackMode java/net/MulticastSocket/Test.java: IPv4 and IPv6 multicasting broken on Linux And 1 test that had error trying to be run: JT Harness : Tests that had errors java/net/URLPermission/nstest/look
Re: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)
Hi Peter,
perhaps the synchronized(this)-block in NameServiceAddresses::get()
can be replaced with a ReentrantLock? Applying this on top of your
patch, I seem to improve scores on your benchmark for -t 4 by ~33% on my
machine:
--- a/src/share/classes/java/net/InetAddress.javaWed Jul 02 14:47:40
2014 +0200
+++ b/src/share/classes/java/net/InetAddress.javaWed Jul 02 14:57:24
2014 +0200
@@ -42,6 +42,7 @@
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.ReentrantLock;
import sun.security.action.*;
import sun.net.InetAddressCachePolicy;
@@ -763,6 +764,7 @@
private static final class NameServiceAddresses implements Addresses {
private final String host;
private final InetAddress reqAddr;
+private final ReentrantLock lock = new ReentrantLock();
NameServiceAddresses(String host, InetAddress reqAddr) {
this.host = host;
@@ -774,7 +776,8 @@
Addresses addresses;
// only one thread is doing lookup to name service
// for particular host at any time.
-synchronized (this) {
+lock.lock();
+try {
// re-check that we are still us + re-install us if
slot empty
addresses = cache.putIfAbsent(host, this);
if (addresses == null) {
@@ -822,10 +825,13 @@
return inetAddresses;
}
// else addresses != this
+
+// delegate to different addresses when we are already
replaced
+// but outside of synchronized block to avoid any
chance of dead-locking
+return addresses.get();
+} finally {
+lock.unlock();
}
-// delegate to different addresses when we are already replaced
-// but outside of synchronized block to avoid any chance of
dead-locking
-return addresses.get();
}
}
nit: line 1258: getAddressesFromNameService made package private but
not used outside of InetAddress?
Generally I think this looks good, but I'm not a reviewer. :)
/Claes
On 07/02/2014 01:56 PM, Peter Levart wrote:
Hi,
I updated the webrev with first two suggestions from Bernd (expireTime
instead of createTime and cacheNanos + only use putIfAbsent instead of
get followed by putIfAbsent):
http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.02/
Thanks, Bernd.
The id field in CachedAddresses is necessary for compareTo to never
return 0 for two different instances (used as element in
ConcurrentSkipListSet).
For last two suggestions I'm not sure whether they are desired, so I'm
currently leaving them as is.
Regards, Peter
On 07/01/2014 10:06 PM, Bernd Eckenfels wrote:
Looks good, like it, Peter.
some nits: instead of adding createTime and cacheNanos, only have a
expireAfter?
L782: is it better to use putIfAbsent unconditionally, instead of
get/putIfAbsent in NameServicdeAddr?
L732: I am unsure about the id field, isnt it enough to have the
identity equality check for the replacement check and otherwise depend
on equals()?
L1223: What about moving the cache exiry inside the if (useCache)
BTW1: might be the wrong RFR, but considering your good performance
numbers for an active cache, would having 100ms or similiar default
negative cache time make sense without impacting (visible) the semantic.
Gruss
Bernd
Am Tue, 01 Jul 2014 20:35:57 +0200
schrieb Peter Levart :
Hi,
I propose a patch for this issue:
https://bugs.openjdk.java.net/browse/JDK-7186258
The motivation to re-design caching of InetAddress-es was not this
issue though, but a desire to attack synchronization bottlenecks in
methods like URL.equals and URL.hashCode which use host name to IP
address mapping. I plan to tackle the synchronization in URL in a
follow-up proposal, but I wanted to 1st iron-out the "leaves" of the
call-tree. Here's the proposed patch:
http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.01/
sun.net.InetAddressCachePolicy:
- two static methods (get() and getNegative()) were synchronized.
Removed synchronization and made underlying fields volatile.
- also added a normalization of negative policy in
setNegativeIfNotSet(). The logic in InetAddress doesn't cope with
negative values distinct from InetAddressCachePolicy.FOREVER (-1), so
this was a straight bug. The setIfNotSet() doesn't need this
normalization, because checkValue() throws exception if passed-in
value < InetAddressCachePolicy.FOREVER.
java.net.InetAddress:
- complete redesign of caching. Instead of distinct Positive/Negative
caches, there's only one cache - a ConcurrentHashMap. The value in
the map knows if it contains positive or negative answer.
- the design of this cache is similar but much simpler than
java.lang.reflect.WeakCache, s
Re: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)
Hi Claes,
Thank you for looking into this patch.
On 07/02/2014 04:39 PM, Claes Redestad wrote:
Hi Peter,
perhaps the synchronized(this)-block in NameServiceAddresses::get()
can be replaced with a ReentrantLock? Applying this on top of your
patch, I seem to improve scores on your benchmark for -t 4 by ~33% on
my machine:
Which test? I would be surprised that this change has any impact but on
"getByNameNegative" test which exhibits this lock at each iteration
(since negative caching is disabled by default). I'll check this myself too.
--- a/src/share/classes/java/net/InetAddress.javaWed Jul 02
14:47:40 2014 +0200
+++ b/src/share/classes/java/net/InetAddress.javaWed Jul 02
14:57:24 2014 +0200
@@ -42,6 +42,7 @@
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.ReentrantLock;
import sun.security.action.*;
import sun.net.InetAddressCachePolicy;
@@ -763,6 +764,7 @@
private static final class NameServiceAddresses implements
Addresses {
private final String host;
private final InetAddress reqAddr;
+private final ReentrantLock lock = new ReentrantLock();
NameServiceAddresses(String host, InetAddress reqAddr) {
this.host = host;
@@ -774,7 +776,8 @@
Addresses addresses;
// only one thread is doing lookup to name service
// for particular host at any time.
-synchronized (this) {
+lock.lock();
+try {
// re-check that we are still us + re-install us if
slot empty
addresses = cache.putIfAbsent(host, this);
if (addresses == null) {
@@ -822,10 +825,13 @@
return inetAddresses;
}
// else addresses != this
+
+// delegate to different addresses when we are
already replaced
+// but outside of synchronized block to avoid any
chance of dead-locking
+return addresses.get();
+} finally {
+lock.unlock();
}
-// delegate to different addresses when we are already
replaced
-// but outside of synchronized block to avoid any chance
of dead-locking
-return addresses.get();
}
}
nit: line 1258: getAddressesFromNameService made package private but
not used outside of InetAddress?
Good catch. I seem to have inadvertently deleted the "private" keyword.
Generally I think this looks good, but I'm not a reviewer. :)
/Claes
Regards, Peter
On 07/02/2014 01:56 PM, Peter Levart wrote:
Hi,
I updated the webrev with first two suggestions from Bernd
(expireTime instead of createTime and cacheNanos + only use
putIfAbsent instead of get followed by putIfAbsent):
http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.02/
Thanks, Bernd.
The id field in CachedAddresses is necessary for compareTo to never
return 0 for two different instances (used as element in
ConcurrentSkipListSet).
For last two suggestions I'm not sure whether they are desired, so
I'm currently leaving them as is.
Regards, Peter
On 07/01/2014 10:06 PM, Bernd Eckenfels wrote:
Looks good, like it, Peter.
some nits: instead of adding createTime and cacheNanos, only have a
expireAfter?
L782: is it better to use putIfAbsent unconditionally, instead of
get/putIfAbsent in NameServicdeAddr?
L732: I am unsure about the id field, isnt it enough to have the
identity equality check for the replacement check and otherwise depend
on equals()?
L1223: What about moving the cache exiry inside the if (useCache)
BTW1: might be the wrong RFR, but considering your good performance
numbers for an active cache, would having 100ms or similiar default
negative cache time make sense without impacting (visible) the
semantic.
Gruss
Bernd
Am Tue, 01 Jul 2014 20:35:57 +0200
schrieb Peter Levart :
Hi,
I propose a patch for this issue:
https://bugs.openjdk.java.net/browse/JDK-7186258
The motivation to re-design caching of InetAddress-es was not this
issue though, but a desire to attack synchronization bottlenecks in
methods like URL.equals and URL.hashCode which use host name to IP
address mapping. I plan to tackle the synchronization in URL in a
follow-up proposal, but I wanted to 1st iron-out the "leaves" of the
call-tree. Here's the proposed patch:
http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.01/
sun.net.InetAddressCachePolicy:
- two static methods (get() and getNegative()) were synchronized.
Removed synchronization and made underlying fields volatile.
- also added a normalization of negative policy in
setNegativeIfNotSet(). The logic in InetAddress doesn't cope with
negative values distinct from InetAddressCachePolicy.FOREVER (-1), so
this was a straight bug. The setIfNotSet() doesn't ne
Re: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)
On 07/02/2014 05:30 PM, Peter Levart wrote:
Hi Claes,
Thank you for looking into this patch.
On 07/02/2014 04:39 PM, Claes Redestad wrote:
Hi Peter,
perhaps the synchronized(this)-block in NameServiceAddresses::get()
can be replaced with a ReentrantLock? Applying this on top of your
patch, I seem to improve scores on your benchmark for -t 4 by ~33% on
my machine:
Which test? I would be surprised that this change has any impact but
on "getByNameNegative" test which exhibits this lock at each iteration
(since negative caching is disabled by default). I'll check this
myself too.
Feel free to ignore this. I don't seem to be able to benchmark properly
today and was jumping to conclusions: rerunning with just a bit more
rigor did not reproduce the improvement. In fact it doesn't even seem to
benefit getByNameNegative. Perhaps you'll get different results.
Thanks.
/Claes
--- a/src/share/classes/java/net/InetAddress.javaWed Jul 02
14:47:40 2014 +0200
+++ b/src/share/classes/java/net/InetAddress.javaWed Jul 02
14:57:24 2014 +0200
@@ -42,6 +42,7 @@
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.ReentrantLock;
import sun.security.action.*;
import sun.net.InetAddressCachePolicy;
@@ -763,6 +764,7 @@
private static final class NameServiceAddresses implements
Addresses {
private final String host;
private final InetAddress reqAddr;
+private final ReentrantLock lock = new ReentrantLock();
NameServiceAddresses(String host, InetAddress reqAddr) {
this.host = host;
@@ -774,7 +776,8 @@
Addresses addresses;
// only one thread is doing lookup to name service
// for particular host at any time.
-synchronized (this) {
+lock.lock();
+try {
// re-check that we are still us + re-install us if
slot empty
addresses = cache.putIfAbsent(host, this);
if (addresses == null) {
@@ -822,10 +825,13 @@
return inetAddresses;
}
// else addresses != this
+
+// delegate to different addresses when we are
already replaced
+// but outside of synchronized block to avoid any
chance of dead-locking
+return addresses.get();
+} finally {
+lock.unlock();
}
-// delegate to different addresses when we are already
replaced
-// but outside of synchronized block to avoid any chance
of dead-locking
-return addresses.get();
}
}
nit: line 1258: getAddressesFromNameService made package private but
not used outside of InetAddress?
Good catch. I seem to have inadvertently deleted the "private" keyword.
Generally I think this looks good, but I'm not a reviewer. :)
/Claes
Regards, Peter
On 07/02/2014 01:56 PM, Peter Levart wrote:
Hi,
I updated the webrev with first two suggestions from Bernd
(expireTime instead of createTime and cacheNanos + only use
putIfAbsent instead of get followed by putIfAbsent):
http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.02/
Thanks, Bernd.
The id field in CachedAddresses is necessary for compareTo to never
return 0 for two different instances (used as element in
ConcurrentSkipListSet).
For last two suggestions I'm not sure whether they are desired, so
I'm currently leaving them as is.
Regards, Peter
On 07/01/2014 10:06 PM, Bernd Eckenfels wrote:
Looks good, like it, Peter.
some nits: instead of adding createTime and cacheNanos, only have a
expireAfter?
L782: is it better to use putIfAbsent unconditionally, instead of
get/putIfAbsent in NameServicdeAddr?
L732: I am unsure about the id field, isnt it enough to have the
identity equality check for the replacement check and otherwise depend
on equals()?
L1223: What about moving the cache exiry inside the if (useCache)
BTW1: might be the wrong RFR, but considering your good performance
numbers for an active cache, would having 100ms or similiar default
negative cache time make sense without impacting (visible) the
semantic.
Gruss
Bernd
Am Tue, 01 Jul 2014 20:35:57 +0200
schrieb Peter Levart :
Hi,
I propose a patch for this issue:
https://bugs.openjdk.java.net/browse/JDK-7186258
The motivation to re-design caching of InetAddress-es was not this
issue though, but a desire to attack synchronization bottlenecks in
methods like URL.equals and URL.hashCode which use host name to IP
address mapping. I plan to tackle the synchronization in URL in a
follow-up proposal, but I wanted to 1st iron-out the "leaves" of the
call-tree. Here's the proposed patch:
http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.01/
sun.net.InetAddressCachePolicy:
- two static methods
Re: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)
Hi Peter, Thanks for contributing this. I've started to review it and will get back to you with comments later in the week. Regards, Michael. On 01/07/14 19:35, Peter Levart wrote: Hi, I propose a patch for this issue: https://bugs.openjdk.java.net/browse/JDK-7186258 The motivation to re-design caching of InetAddress-es was not this issue though, but a desire to attack synchronization bottlenecks in methods like URL.equals and URL.hashCode which use host name to IP address mapping. I plan to tackle the synchronization in URL in a follow-up proposal, but I wanted to 1st iron-out the "leaves" of the call-tree. Here's the proposed patch: http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.01/ sun.net.InetAddressCachePolicy: - two static methods (get() and getNegative()) were synchronized. Removed synchronization and made underlying fields volatile. - also added a normalization of negative policy in setNegativeIfNotSet(). The logic in InetAddress doesn't cope with negative values distinct from InetAddressCachePolicy.FOREVER (-1), so this was a straight bug. The setIfNotSet() doesn't need this normalization, because checkValue() throws exception if passed-in value < InetAddressCachePolicy.FOREVER. java.net.InetAddress: - complete redesign of caching. Instead of distinct Positive/Negative caches, there's only one cache - a ConcurrentHashMap. The value in the map knows if it contains positive or negative answer. - the design of this cache is similar but much simpler than java.lang.reflect.WeakCache, since it doesn't have to deal with WeakReferences and keys are simpler (just strings - hostnames). Similarity is in how concurrent requests for the same key (hostname) are synchronized when the entry is not cached yet, but still avoid synchronization when entry is cached. This preserves the behaviour of original InetAddress caching code but simplifies it greatly (100+ lines removed). - I tried to preserve the interaction between InetAddress.getLocalHost() and InetAddress.getByName(). The getLocalHost() caches the local host address for 5 seconds privately. When it expires it performs new name service look-up and "refreshes" the entry in the InetAddress.getByName() cache although it has not expired yet. I think this is meant to prevent surprises when getLocalHost() returns newer address than getByName() which is called after that. - I also fixed the JDK-7186258 as a by-product (but don't know yet how to write a test for this issue - any ideas?) I created a JMH benchmark that tests the following methods: - InetAddress.getLocalHost() - InetAddress.getByName() (with positive and negative answer) Here're the results of running on my 4-core (8-threads) i7/Linux: http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/InetAddress.Cache_bench_results.01.pdf The getByNameNegative() test does not show much improvement in patched vs. original code. That's because by default the policy is to NOT cache negative answers. Requests for same hostname to the NameService(s) are synchronized. If "networkaddress.cache.negative.ttl" system property is set to some positive value, results are similar to those of getByNamePositive() test (the default policy for positive caching is 30 seconds). I ran the jtreg tests in test/java/net and have the same score as with original unpatched code. I have 3 failing tests from original and patched runs: JT Harness : Tests that failed java/net/MulticastSocket/Promiscuous.java: Test for interference when two sockets are bound to the same port but joined to different multicast groups java/net/MulticastSocket/SetLoopbackMode.java: Test MulticastSocket.setLoopbackMode java/net/MulticastSocket/Test.java: IPv4 and IPv6 multicasting broken on Linux And 1 test that had error trying to be run: JT Harness : Tests that had errors java/net/URLPermission/nstest/lookup.sh: Because of: test result: Error. Can't find source file: jdk/testlibrary/*.java in directory-list: /home/peter/work/hg/jdk9-dev/jdk/test/java/net/URLPermission/nstest /home/peter/work/hg/jdk9-dev/jdk/test/lib/testlibrary All other 258 java/net tests pass. So what do you think? Regards, Peter
Re: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)
Hello Peter,
I think the comments in compareTo() are now superflucious ("with 0").
Greetings
Bernd
Am Wed, 02 Jul 2014 13:56:39 +0200
schrieb Peter Levart :
> Hi,
>
> I updated the webrev with first two suggestions from Bernd
> (expireTime instead of createTime and cacheNanos + only use
> putIfAbsent instead of get followed by putIfAbsent):
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.02/
