Re: RFR JDK-8049228: Improve multithreaded scalability of InetAddress cache

[Peter Levart]

Hi Michael,

I have re-based the patch to the new jdk9 source layout. Nothing changes 
from the webrev.03 except paths:

http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.04/


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.lev...@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