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.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/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
test result: Error. Can't find source file: jdk/testlibrary/*.java
All other 258 java/net tests pass.
So what do you think?