RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)
Claes Redestad
claes.redestad at oracle.com
Wed Jul 2 14:39:51 UTC 2014
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.java Wed Jul 02 14:47:40
2014 +0200
+++ b/src/share/classes/java/net/InetAddress.java Wed 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 <peter.levart at gmail.com>:
>>
>>> 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
>>>
>>>
>
More information about the net-dev
mailing list