RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)
Claes Redestad
claes.redestad at oracle.com
Wed Jul 2 15:57:54 UTC 2014
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.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?
>
> 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 <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 core-libs-dev
mailing list