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