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

[Michael McMahon]

Hi Peter,

Thanks for contributing this. I've started to review it
and will get back to you with comments later in the week.

Regards,
Michael.

On 01/07/14 19:35, Peter Levart wrote:
> 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/20140702/bc68ca31/attachment-0001.html>