RFR JDK-8049228: Improve multithreaded scalability of InetAddress cache

Tue Aug 19 10:51:11 UTC 2014

Hi Michael,

I have re-based the patch to the new jdk9 source layout. Nothing changes 
from the webrev.03 except paths:

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

Regards, Peter

On 07/09/2014 01:52 PM, Peter Levart wrote:
> 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
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>