RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)

[Peter Levart]

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