RFR JDK-8049228: Improve multithreaded scalability of InetAddress cache (was: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more))

Wed Jul 9 11:52:37 UTC 2014

Hi Michael,

Thanks for testing. I have prepared another webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.03/

It only cleans up two comments suggested by Bernd (removed superfluous 
phrase "with 0" from statements about comparing time instants). So do 
you think this needs more testing / another review or can I consider 
this reviewed for jdk9-dev ?

Regards, Peter

On 07/07/2014 04:13 PM, Michael McMahon wrote:
> Peter,
>
> Thanks for the explanation. No. I think your change is good. I've run 
> tests here locally
> and I'm happy with it overall.
>
> Michael
>
> On 07/07/14 14:10, Peter Levart wrote:
>> On 07/07/2014 12:59 PM, Michael McMahon wrote:
>>> Hi Peter,
>>>
>>> Is it necessary to remove the cache entry in the local host case 
>>> (L1226) ?
>>> It seems redundant to cache it here, and also explicitly in the 
>>> CachedLocalHost object
>>>
>>> Michael
>>
>> Hi Michael,
>>
>> Thanks for looking into this.
>>
>> getLocalHost() seems to have a special hard-coded policy of positive 
>> caching for 5 seconds that is independent of general getByName() 
>> caching policy (30 seconds by default). The behaviour of original 
>> code that I'm trying to replicate is such that when getLocalHost() 
>> notices a change of local host name -> address mapping, the mapping 
>> in global cache for this change is also updated. I think this is to 
>> avoid situations like:
>>
>> Let's say the local host name is "cube":
>>
>> InetAddress addr1 = InetAddress.getLocalHost();
>> InetAddress addr2 = InetAddress.getByName("cube");
>> // addr1.equals(addr2) here
>>
>> // 5 seconds later, cube -> IP mapping is updated in DNS or 
>> /etc/hosts ...
>>
>> addr1 = InetAddress.getLocalHost();
>> addr2 = InetAddress.getByName("cube");
>> // if getLocalHost() did not update global cache,
>> // addr1 (new IP address) would be different from addr2 (old IP address)
>>
>>
>> Another way to accomplish similar guarantee would be to special-case 
>> the caching policy in global cache which would check whether the 
>> entry is for local host name and set 'expiryTime' accordingly. This 
>> would be a little different behaviourally, because 
>> InetAddress.getByName() would get a 5 second expiry for local host 
>> name too, regardless of whether InetAddress.getLocalHost() has been 
>> called at all. But we could get rid of special CachedLocalHost class 
>> then. Is such behavioural change warranted?
>>
>> Regards, Peter
>>
>>>
>>> On 02/07/14 12:56, 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
>>>>>>
>>>>>>
>>>>
>>>
>>
>