RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)
peter.levart at gmail.com
Wed Jul 2 11:56:39 UTC 2014
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):
The id field in CachedAddresses is necessary for compareTo to never
return 0 for two different instances (used as element in
For last two suggestions I'm not sure whether they are desired, so I'm
currently leaving them as is.
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
> 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.
> Am Tue, 01 Jul 2014 20:35:57 +0200
> schrieb Peter Levart <peter.levart at gmail.com>:
>> I propose a patch for this issue:
>> 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:
>> - 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.
>> - 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:
>> 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
>> 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
>> Because of:
>> test result: Error. Can't find source file: jdk/testlibrary/*.java in
>> All other 258 java/net tests pass.
>> So what do you think?
>> Regards, Peter
More information about the core-libs-dev