Hi Bernd,
Thanks for reviewing.
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?
I could, yes. I just have to be careful not co compare two expireAfter
values with '<' or '>'.
The javadoc for System.nanoTime() states:
"
To compare two nanoTime values
long t0 = System.nanoTime();
...
long t1 = System.nanoTime();
one should use t1 - t0 < 0, not t1 < t0, because of the possibility of
numerical overflow.
"
The thing is that one never knows with System.nanoTime() when its start
(value 0) is. It's not defined. So a later time instant can be negative
while an earlier one can be positive. But I could compare two instants
which are no more than 290 years apart with no problem using (t1 - t2)
<=> 0 . I'll improve this.
L782: is it better to use putIfAbsent unconditionally, instead of
get/putIfAbsent in NameServicdeAddr?
I want to keep the semantics of original code that guarantees that there
will only be a single look-up to the name service per hostname in a
given caching period. If several threads want to look-up the same
hostname at the same time, only one will do it, others will wait and
then use the result of that thread. That's why I re-check the cached
value after entering the synchronized block and act upon what I find
there...
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()?
The ConcurrentSkipListSet (based on ConcurrentSkipListMap) that is used
as an ordered concurrent (but not blocking or locking) queue of items to
be expired is not using hashCode/equals, but compareTo instead. So two
instances of CachedAddresses must never compareTo each other as being
"equal" if one wants to keep them all in the ConcurrentSkipListSet...
L1223: What about moving the cache expiry inside the if (useCache)
That's a possibility. InetAddress.getLocalHost() calls will not expire
cache entries then. There are also other "optimizations" possible. For
example if there is a "storm" of distinct host names to be looked-up,
they enter the cache at about the same time, so they expire at about the
same time too. It could happen that one thread must remove a lot of
expired entries at one time. Perhaps an upper bound on the number of
entries that get removed in one getByName() call could be specified.
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.
It depends on the "application". If some app is doing more than 10
look-ups of same non-existent hostname per second, it will have effect.
Otherwise not. It seems that negative answers are not so frequent in
practice that default policy would require caching them. One can always
choose the policy manually.
Gruss
Bernd
Regards, Peter
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
