RFR JDK-8049228: Improve multithreaded scalability of InetAddress cache

[Peter Levart]

On 07/09/2014 01:54 PM, Claes Redestad wrote:
> I see you're still expanding scope from private to package:
>
> -    private static InetAddress[] getAddressesFromNameService(String 
> host, InetAddress reqAddr)
> +    static InetAddress[] getAddressesFromNameService(String host, 
> InetAddress reqAddr)
>
> /Claes

Thanks for being alert, Claes. I think I own an explanation. After you 
have notified me about this the 1st time, I remembered why I dropped the 
"private" from method declaration in the first place. The method is now 
called from within NameServiceAddresses inner class. I wanted to 
suppress javac from generating synthetic access methods. Since java.net 
is a sealed package, there's no security issue, am I right?

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
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
>