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

[Peter Levart]

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/20140701/97220a97/attachment.html>