RFR JDK-8049228: Improve multithreaded scalability of InetAddress cache (was: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more))

[Peter Levart]

On 07/01/2014 10:04 PM, Martin Buchholz wrote:
> I haven't looked at the patch, but generally ... all uses of
> currentTimeMillis to measure elapsed time should be migrated to use
> difference of two nanoTime values, and such a change should be done
> independently of other changes.

I have filed a RFE that is more suitable for this change:

     https://bugs.openjdk.java.net/browse/JDK-8049228

So, is it strictly necessary to fix JDK-7186258 alone before applying 
the change for JDK-8049228 although it would supersede it?

Regards, Peter

>
> IIRC lookup.sh is a known flaky test being fixed elsewhere.
>
>
> On Tue, Jul 1, 2014 at 11:35 AM, Peter Levart <peter.levart at gmail.com>
> 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
>>
>>