June 2015 - core-libs-dev

Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
by Chris Plummer 05 Jun '15

05 Jun '15

[Adding core-libs-dev(a)openjdk.java.net since this update includes changes to jdk/test library code] Please review the updated webrev: Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054386 There were concerns about the new hotspot tests referencing jdk tests. One concern was that if the jdk tests change, they could break the hotspot tests, and this might initially go undetected. The other concern is that if the jdk and hotspot tests are placed in separate test bundles, then it would not be possible to run the hotspot tests. Because of these concerns, I moved the tests that were in hotspot/test/runtime/CDSJDITests to instead be in jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the tests in the process. Also, I had to update the jdk version of ProcessTool.java to include the createJavaProcessBuilder() variant that is in the hotspot version of ProcessTool.java. Lastly, in CDSJITTest.java I changed: OutputAnalyzer output = new OutputAnalyzer(pb.start()); to instead be: OutputAnalyzer output = ProcessTools.executeProcess(pb); I had to do this since the jdk version of the OutputAnalyzer constructor is not public. The 1st version is what is commonly used in hostspot tests, and the 2nd version is what is commonly used in jdk tests. I decided to adopt the jdk way rather than make the OutputAnalyzer constructors public, although this will probably happen eventually when the two versions are unified. thanks, Chris On 5/19/15 7:25 AM, Chris Plummer wrote: > Hi, > > Please review the following changes for allowing java debugging when > CDS is enabled. > > Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/ > Bug:https://bugs.openjdk.java.net/browse/JDK-8054386 > > The VM changes are simple. I removed the check that prevents debugging > with CDS enabled, and added logic that will map the CDS archive RW > when debugging is enabled. > > The tests are a bit more complex. There are a bunch of existing JDI > tests for testing debugging support. Rather than start from scratch or > clone them, I instead just wrote wrapper tests that put the relevant > JDI test classes in the archive, and then invoke the JDI test. I did > this for 3 of the JDI tests. If you feel there are others that would > be good candidates, I'd be happy to add them. I'm looking for ones > that would result in modification of the RO class metadata, such as > setting a breakpoint (for which I already added two tests). > > Testing done: > -Using JPRT to run the new jtreg tests on all platforms. > -Using JPRT to run all jtreg runtime tests on linux x86 and x_64. > -Regular JPRT "-testset hotspot" run > -Putting the JCK JVMTI tests in the archive and then running them. > -Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and > then running them. > -Putting a simple test class in the archive and then setting a > breakpoint on it using jdb > > Some of the above testing resulted in the discovery of bugs that still > need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181. > > I also verified that without the change to map the archive RW, the > above testing resulted in a SEGV, which is what you would expect (and > actually want to see to prove that the testing is effective). > > thanks, > > Chris >

4 6

Lower overhead String encoding/decoding
by Richard Warburton 04 Jun '15

04 Jun '15

Hi all, A long-standing issue with Strings in Java is the ease and performance of creating a String from a ByteBuffer. People who are using nio to bring in data off the network will be receiving that data in the form of bytebuffers and converting it to some form of String. For example restful systems receiving XML or Json messages. The current workaround is to create a byte[] from the ByteBuffer - a copying action for any direct bytebuffer - and then pass that to the String. I'd like to propose that we add an additional constructor to the String class that takes a ByteBuffer as an argument, and directly create the char[] value inside the String from the ByteBuffer. Similarly if you have a String that you want to encode onto the wire then you need to call String.getBytes(), then write your byte[] into a ByteBuffer or send it over the network. This ends up allocating a byte[] to do the copy and also trimming the byte[] back down again, usually allocating another byte[]. To address this problem I've added a couple of getBytes() overloads that take byte[] and ByteBuffer arguments and write directly to those buffers. I've put together a patch that implements this to demonstrate the overall direction. http://cr.openjdk.java.net/~rwarburton/string-patch-webrev-5/ I'm happy to take any feedback on direction or the patch itself or the overall idea/approach. I think there are a number of similar API situations in other places as well, for example StringBuffer/StringBuilder instances which could have similar appends directly from ByteBuffer instances instead of byte[] instances. I'll also be at Javaone next week, so if you want to talk about this, just let me know. regards, Richard Warburton http://insightfullogic.com @RichardWarburto <http://twitter.com/richardwarburto> PS: I appreciate that since I'm adding a method to the public API which consequently requires standardisation but I think that this could get incorporated into the Java 9 umbrella JSR.

7 27

RFR [9]: Better failure output for test/java/util/Arrays/ParallelPrefix.java
by Chris Hegarty 04 Jun '15

04 Jun '15

This is a review request to add better test failure output to jdk/test/java/util/Arrays/ParallelPrefix.java This test has been seen to fail occasionally on recent JDK 9 builds. It fails with a message similar to: ... test ParallelPrefix.testParallelPrefixForInt([I@47a7a8ef, 1365, 2730, ParallelPrefix$$Lambda$56/1605520928@106dbc48): success test ParallelPrefix.testParallelPrefixForInt([I@789e465e, 1365, 2731, ParallelPrefix$$Lambda$55/1326195546@2c8625ae): failure java.lang.AssertionError: expected [[I@7b703d4a] but found [[I@73625cb4] at org.testng.Assert.fail(Assert.java:94) at org.testng.Assert.failNotEquals(Assert.java:494) at org.testng.Assert.assertArrayEquals(Assert.java:144) at org.testng.Assert.assertEquals(Assert.java:117) at org.testng.Assert.assertEquals(Assert.java:165) at ParallelPrefix.testParallelPrefixForInt(ParallelPrefix.java:124) … The output is not very helpful. Testng, or at least the version that we have in use in jtreg, does not provide pretty failure messages when dealing with arrays. It is trivial to add these to the test, see below. Note: I used the 3-args version of assertEquals as it will trigger the inclusion of the index of the array where the values differ. Hopefully, when we see this test fail again it can produce some meaningful output. -Chris. diff --git a/test/java/util/Arrays/ParallelPrefix.java b/test/java/util/Arrays/ParallelPrefix.java --- a/test/java/util/Arrays/ParallelPrefix.java +++ b/test/java/util/Arrays/ParallelPrefix.java @@ -39,6 +39,7 @@ import static org.testng.Assert.*; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; +import static java.lang.System.out; public class ParallelPrefix { //Array size less than MIN_PARTITION @@ -121,11 +122,11 @@ int[] parallelResult = data.clone(); Arrays.parallelPrefix(parallelResult, fromIndex, toIndex, op); - assertEquals(parallelResult, sequentialResult); + assertArraysEqual(parallelResult, sequentialResult); int[] parallelRangeResult = Arrays.copyOfRange(data, fromIndex, toIndex); Arrays.parallelPrefix(parallelRangeResult, op); - assertEquals(parallelRangeResult, Arrays.copyOfRange(sequentialResult, fromIndex, toIndex)); + assertArraysEqual(parallelRangeResult, Arrays.copyOfRange(sequentialResult, fromIndex, toIndex)); } @Test(dataProvider="longSet") @@ -137,11 +138,11 @@ long[] parallelResult = data.clone(); Arrays.parallelPrefix(parallelResult, fromIndex, toIndex, op); - assertEquals(parallelResult, sequentialResult); + assertArraysEqual(parallelResult, sequentialResult); long[] parallelRangeResult = Arrays.copyOfRange(data, fromIndex, toIndex); Arrays.parallelPrefix(parallelRangeResult, op); - assertEquals(parallelRangeResult, Arrays.copyOfRange(sequentialResult, fromIndex, toIndex)); + assertArraysEqual(parallelRangeResult, Arrays.copyOfRange(sequentialResult, fromIndex, toIndex)); } @Test(dataProvider="doubleSet") @@ -153,11 +154,11 @@ double[] parallelResult = data.clone(); Arrays.parallelPrefix(parallelResult, fromIndex, toIndex, op); - assertEquals(parallelResult, sequentialResult); + assertArraysEqual(parallelResult, sequentialResult); double[] parallelRangeResult = Arrays.copyOfRange(data, fromIndex, toIndex); Arrays.parallelPrefix(parallelRangeResult, op); - assertEquals(parallelRangeResult, Arrays.copyOfRange(sequentialResult, fromIndex, toIndex)); + assertArraysEqual(parallelRangeResult, Arrays.copyOfRange(sequentialResult, fromIndex, toIndex)); } @Test(dataProvider="stringSet") @@ -169,11 +170,11 @@ String[] parallelResult = data.clone(); Arrays.parallelPrefix(parallelResult, fromIndex, toIndex, op); - assertEquals(parallelResult, sequentialResult); + assertArraysEqual(parallelResult, sequentialResult); String[] parallelRangeResult = Arrays.copyOfRange(data, fromIndex, toIndex); Arrays.parallelPrefix(parallelRangeResult, op); - assertEquals(parallelRangeResult, Arrays.copyOfRange(sequentialResult, fromIndex, toIndex)); + assertArraysEqual(parallelRangeResult, Arrays.copyOfRange(sequentialResult, fromIndex, toIndex)); } @Test @@ -293,5 +294,41 @@ public static void assertInstance(Object actual, Class<?> expected, String message) { assertTrue(expected.isInstance(actual), message); } + + static void assertArraysEqual(int[] actual, int[] expected) { + try { + assertEquals(actual, expected, ""); + } catch (AssertionError x) { + out.printf("Expected:%s, actual:%s%n", Arrays.toString(expected), Arrays.toString(actual)); + throw x; + } + } + + static void assertArraysEqual(long[] actual, long[] expected) { + try { + assertEquals(actual, expected, ""); + } catch (AssertionError x) { + out.printf("Expected:%s, actual:%s%n", Arrays.toString(expected), Arrays.toString(actual)); + throw x; + } + } + + static void assertArraysEqual(double[] actual, double[] expected) { + try { + assertEquals(actual, expected, ""); + } catch (AssertionError x) { + out.printf("Expected:%s, actual:%s%n", Arrays.toString(expected), Arrays.toString(actual)); + throw x; + } + } + + static void assertArraysEqual(String[] actual, String[] expected) { + try { + assertEquals(actual, expected, ""); + } catch (AssertionError x) { + out.printf("Expected:%s, actual:%s%n", Arrays.toString(expected), Arrays.toString(actual)); + throw x; + } + } }

3 5

[8u-dev] Review request : JDK-8068416: LFGarbageCollectedTest.java fails with OOME: "GC overhead limit exceeded"
by Konstantin Shefov 04 Jun '15

04 Jun '15

Hello, Please review the test bug fix https://bugs.openjdk.java.net/browse/JDK-8068416 Webrev is http://cr.openjdk.java.net/~kshefov/8068416/webrev.00/ Test fails only against JDK 8u and passes against JDK 9. Fix is to reduce the number of iterations to 40. With that number of iterations the test passes on those hosts where it failed before. The number of iterations the test start to fail is 65. Before the fix the number of iterations was 84. Thanks -Konstantin

4 11

RFR 8081678: Add Stream returning methods to classes where there currently exist only Enumeration returning methods
by Paul Sandoz 04 Jun '15

04 Jun '15

Hi, Please review a follow up to Stuart's Enumeration.asIterator patch that adds some Stream return methods to classes where there is only Enumeration returning methods to support traversal: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8081678-enumeration-and-stream… I took the opportunity to sprinkle some Enumerator.asIterator impls. I originally included some methods on ClassLoader but Alan wisely warned me away from doing that as this area is likely to be impacted by modularity. There is one small area of uncertainty with NetworkInterface. Can the following method ever return null? 342 public static Enumeration<NetworkInterface> getNetworkInterfaces() 343 throws SocketException { 344 NetworkInterface[] netifs = getAll(); 345 346 // specified to return null if no network interfaces 347 return netifs != null 348 ? enumerationFromArray(netifs) 349 : null; 350 } Contrary to the comment i cannot find any specification. For the stream returning method, networkInterfaces, i have specified this to return an empty stream, thus it might be good to update the enumeration returning method as well to say whether it returns null or an empty enumeration. Paul.

4 11

[9] Review request for 8072515: Test Task: Develop new tests for JEP 219: Datagram Transport Layer Security (DTLS)
by Konstantin Shefov 04 Jun '15

04 Jun '15

Hello, Please review new tests fro DTLS feature for JDK 9: bug: https://bugs.openjdk.java.net/browse/JDK-8072515 webrev: http://cr.openjdk.java.net/~kshefov/8072515/webrev.00/ Thanks -Konstantin

2 2

[9] RFR (XS) JDK-8081771: ProcessTool.createJavaProcessBuilder() needs new addTestVmAndJavaOptions argument
by Chris Plummer 04 Jun '15

04 Jun '15

Please review the following: Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/ Bug: https://bugs.openjdk.java.net/browse/JDK-8081771 This review only concerns the changes to ProcessTool.java. The CDSJDITests and filemapp.cpp changes will be committed under CR JDK-8054386, but they rely on this change to ProcessTool.java, so both CRs will be pushed together. See the following thread for the JDK-8054386 review: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-June/014923… thanks, Chris

4 4

Why isn't Object.notify() a synchronized method?
by Ulf Zibis 04 Jun '15

04 Jun '15

Hi all, in the Javadoc of notify(), notifyAll() and wait(...) I read, that this methods should only be used with synchronisation on it's instance. So I'm wondering, why they don't have the synchronized modifier out of the box in Object class. Also I think, the following note should be moved from wait(long,int) to wait(long): /The current thread must own this object's monitor. The thread releases ownership of this monitor and waits until either of the following two conditions has occurred:// / * /Another thread notifies threads waiting on this object's monitor to wake up either through a call to the notify method or the notifyAll method./ * /The timeout period, specified by timeout milliseconds plus nanos nanoseconds arguments, has elapsed. / Cheers, Ulf

7 8

Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled
by serguei.spitsyn＠oracle.com 04 Jun '15

04 Jun '15

Chris, It looks good in general. I'd suggest to rename the folder: || test/com/sun/jdi/CDSJDITests to: test/com/sun/jdi/cds There is no need to spell "JDI" as it is already a sub-folder of the com/sun/jdi and there is no need to spell "Tests" too as it is in the test repo. Also, all the folders are normally named in the lower case. Thanks, Serguei On 6/2/15 8:25 PM, Chris Plummer wrote: > Ok, I'm going to keep this as one webrev, but I did create JDK-8081771 > for the ProcessTool.java changes: > > https://bugs.openjdk.java.net/browse/JDK-8081771 > > I will commit ProcessTool.java under JDK-8081771, and the rest of the > changes under JDK-8054386. Both will then be pushed together. I also > started a new thread for the review of JDK-8081771: > > http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-June/014930… > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html > > thanks, > > Chris > > On 6/2/15 11:55 AM, Chris Plummer wrote: >> I'm going to have to separate out the ProcessTool.java changes into a >> separate changeset (and CR). In the meantime, feel free to review >> what I have below. The code won't be changing at all when I separate >> out the ProcessTool.java changes. >> >> thanks, >> >> Chris >> >> On 6/2/15 12:36 AM, Chris Plummer wrote: >>> [Adding core-libs-dev(a)openjdk.java.net since this update includes >>> changes to jdk/test library code] >>> >>> Please review the updated webrev: >>> >>> Webrev: http://cr.openjdk.java.net/~cjplummer/8054386/webrev.02/ >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8054386 >>> >>> There were concerns about the new hotspot tests referencing jdk >>> tests. One concern was that if the jdk tests change, they could >>> break the hotspot tests, and this might initially go undetected. The >>> other concern is that if the jdk and hotspot tests are placed in >>> separate test bundles, then it would not be possible to run the >>> hotspot tests. >>> >>> Because of these concerns, I moved the tests that were in >>> hotspot/test/runtime/CDSJDITests to instead be in >>> jdk/test/com/sun/jdi/CDSJDITests. There was a slight renaming of the >>> tests in the process. Also, I had to update the jdk version of >>> ProcessTool.java to include the createJavaProcessBuilder() variant >>> that is in the hotspot version of ProcessTool.java. >>> >>> Lastly, in CDSJITTest.java I changed: >>> >>> OutputAnalyzer output = new OutputAnalyzer(pb.start()); >>> >>> to instead be: >>> >>> OutputAnalyzer output = ProcessTools.executeProcess(pb); >>> >>> I had to do this since the jdk version of the OutputAnalyzer >>> constructor is not public. The 1st version is what is commonly used >>> in hostspot tests, and the 2nd version is what is commonly used in >>> jdk tests. I decided to adopt the jdk way rather than make the >>> OutputAnalyzer constructors public, although this will probably >>> happen eventually when the two versions are unified. >>> >>> thanks, >>> >>> Chris >>> >>> >>> On 5/19/15 7:25 AM, Chris Plummer wrote: >>>> Hi, >>>> >>>> Please review the following changes for allowing java debugging >>>> when CDS is enabled. >>>> >>>> Webrev:http://cr.openjdk.java.net/~cjplummer/8054386/webrev.01/ >>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8054386 >>>> >>>> The VM changes are simple. I removed the check that prevents >>>> debugging with CDS enabled, and added logic that will map the CDS >>>> archive RW when debugging is enabled. >>>> >>>> The tests are a bit more complex. There are a bunch of existing JDI >>>> tests for testing debugging support. Rather than start from scratch >>>> or clone them, I instead just wrote wrapper tests that put the >>>> relevant JDI test classes in the archive, and then invoke the JDI >>>> test. I did this for 3 of the JDI tests. If you feel there are >>>> others that would be good candidates, I'd be happy to add them. I'm >>>> looking for ones that would result in modification of the RO class >>>> metadata, such as setting a breakpoint (for which I already added >>>> two tests). >>>> >>>> Testing done: >>>> -Using JPRT to run the new jtreg tests on all platforms. >>>> -Using JPRT to run all jtreg runtime tests on linux x86 and x_64. >>>> -Regular JPRT "-testset hotspot" run >>>> -Putting the JCK JVMTI tests in the archive and then running them. >>>> -Putting the nsk jdb, jdwp, jvmti, and jdi tests in the archive and >>>> then running them. >>>> -Putting a simple test class in the archive and then setting a >>>> breakpoint on it using jdb >>>> >>>> Some of the above testing resulted in the discovery of bugs that >>>> still need to be addressed: JDK-8078644, JDK-8078730, and JDK-8079181. >>>> >>>> I also verified that without the change to map the archive RW, the >>>> above testing resulted in a SEGV, which is what you would expect >>>> (and actually want to see to prove that the testing is effective). >>>> >>>> thanks, >>>> >>>> Chris >>>> >>> >> >

2 2

RFR 9: 8081567 : java/lang/ProcessHandle/InfoTest.java failed Cannot run program "whoami"
by Roger Riggs 03 Jun '15

03 Jun '15

Please review a change to report the user/owner in ProcessHandle.info to have the same form for the owner identification as provided by java.nio.Files. On Windows it includes the domain with the user name. The test is updated to remove a dependency on the OS command whoami and instead compare the users process with the owner of a file created by the process improving portability. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-whoami-8081567/ Issue: https://bugs.openjdk.java.net/browse/JDK-8081567 Thanks, Roger

2 6