RFR JDK-8049228: Improve multithreaded scalability of InetAddress cache

[Peter Levart]

On 08/20/2014 02:47 PM, Michael McMahon wrote:
> This still looks fine to me Peter.
>
> Thanks
> Michael

Hi Michael,

Do I need another reviewer to push this to jdk9-dev ? I don't have a 
clear picture about how many reviewers are needed for what parts of JDK 
code.

Regards, Peter

>
> On 19/08/14 11:51, Peter Levart wrote:
>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20140820/0531146e/attachment-0001.html>