Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled

Hi Chris, Hotspot change is good. Only a couple of style nits with the tests (where are our Java style guidelines ???). Taking CDSJDITest.java as an example: If you are okay with this line length: 115 public static OutputAnalyzer executeAndLog(ProcessBuilder pb, String logName) throws

Re: [9] RFR (XS) JDK-8081771: ProcessTool.createJavaProcessBuilder() needs new addTestVmAndJavaOptions argument

Typo ... On 4/06/2015 4:04 PM, David Holmes wrote: Hi Chris, On 3/06/2015 1:20 PM, Chris Plummer wrote: 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

Re: [9] RFR (XS) JDK-8081771: ProcessTool.createJavaProcessBuilder() needs new addTestVmAndJavaOptions argument

Hi Chris, On 3/06/2015 1:20 PM, Chris Plummer wrote: 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 Your new method needs javado

Re: [9] Review request for 8072515: Test Task: Develop new tests for JEP 219: Datagram Transport Layer Security (DTLS)

Looks fine to me. It's nice to keep each line not exceed 80 characters. For example - * @run main/othervm -Dtest.security.protocol=DTLS -Dtest.mode=norm DTLSBufferOverflowUnderflowTest + * @run main/othervm -Dtest.security.protocol=DTLS + * -Dtest.mode=norm DTLSBufferOverflowUnderflowTest

Re: Why isn't Object.notify() a synchronized method?

The performance issue here is mostly a red herring. The reason notify() is not synchronized has much more to do with correctness; when you "forget" to wrap your notify call with lock acquisition, it is almost always a bug. (The rest of this explanation is probably clearer if you've read JCiP

Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled

Hi Chris, I've replied with a thumbs up on another thread. Thanks, Serguei On 6/3/15 4:23 PM, Chris Plummer wrote: Hi Serguei, I'll take care of the rename. Can I also put you down for the ProcessTool.java changes, which are officially being reviewed on another thread: http://mail.openjd

Re: [9] RFR (XS) JDK-8081771: ProcessTool.createJavaProcessBuilder() needs new addTestVmAndJavaOptions argument

It looks good to me. Reviewed all together. Thanks, Serguei Thanks, Serguei On 6/2/15 8:20 PM, Chris Plummer wrote: 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 th

Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled

Hi Serguei, I'll take care of the rename. Can I also put you down for the ProcessTool.java changes, which are officially being reviewed on another thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-June/033892.html thanks, Chris On 6/3/15 1:40 AM, serguei.spit...@oracle.com

Re: RFR(M, v13): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

> On Jun 3, 2015, at 12:22 PM, Peter Levart wrote: > > > On 06/03/2015 08:36 PM, Mandy Chung wrote: >> >> >> On 06/03/2015 08:29 AM, Dmitry Samersoff wrote: >>> Updated webrev: >>> >>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13 >> >> I reviewed the jdk change. The vers

Re: RFR 9: 8081567 : java/lang/ProcessHandle/InfoTest.java failed Cannot run program "whoami"

Hi Ivan, Thanks for the cleanup suggestions. Roger On 6/3/2015 5:58 PM, Ivan Gerasimov wrote: Thanks! -- test/java/lang/ProcessHandle/InfoTest.java -- Looks good -- src/java.base/windows/native/libjava/ProcessHandleImpl_win.c

Re: RFR 9: 8081567 : java/lang/ProcessHandle/InfoTest.java failed Cannot run program "whoami"

Thanks! -- test/java/lang/ProcessHandle/InfoTest.java -- Looks good -- src/java.base/windows/native/libjava/ProcessHandleImpl_win.c -- The changes look good to me! I guess it does have more sense to use StringSID if the length of u

Re: RFR 8081678: Add Stream returning methods to classes where there currently exist only Enumeration returning methods

Looks good Paul, just a few minor comments. ( looked at all, but *security* and *sql* ) NetworkInterface s/Enumertion/Stream L127 * will be returned in the Enumeration. However, if the caller has the s/an/a L131 * @return an Stream object with all or a subset of the InetAddresses

Re: String.contains(CharSequence) calls toString on argument

Hi, thanks for your feedback. I think it would be more readable here, if you would use if ... else ... . To me a ternary operator is more readable, if there is one value to compute. Also it enhances readability over the whole code, if I use less lines as possible, in other words, I hate to scrol

Re: [8u-dev] Review request : JDK-8068416: LFGarbageCollectedTest.java fails with OOME: "GC overhead limit exceeded"

Konstantin, do you have an explanation why the test passes on jdk 9? from my point of view, it indicates there is a product bug in 8u which should be fixed and your fix just hides it. Igor On 06/03/2015 10:14 PM, Seán Coffey wrote: I bumped into this failure myself today. I think you've got

Re: RFR 8081678: Add Stream returning methods to classes where there currently exist only Enumeration returning methods

On Jun 3, 2015, at 9:27 PM, Alan Bateman wrote: > On 03/06/2015 17:47, Paul Sandoz wrote: >> : >> Ok, i removed it but added an assert for the array being non-null and >> containing at least one element. I also refined the documentation of the >> stream returning method in light of this: >> >

Re: Why isn't Object.notify() a synchronized method?

On Sun, May 31, 2015 at 02:31:25PM +1000, David Holmes wrote: > >As I recently fell into the trap of forgetting the synchronized block > >around a single notifyAll(), I believe, the current situation is just > >errorprone. > > How is it errorprone? You forgot to acquire the lock and you got an > I

Re: RFR 8072773 (fs) Files.lines needs a better splitting implementation for stream source

On Jun 3, 2015, at 9:19 PM, Xueming Shen wrote: > On 06/03/2015 08:53 AM, Paul Sandoz wrote: >> Hi, >> >> Please review an optimization for Files.lines for certain charsets: >> >> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8072773-File-lines/webrev/ >> >> If a charset is say US-ASCII or UT

Re: RFR 8081678: Add Stream returning methods to classes where there currently exist only Enumeration returning methods

On 03/06/2015 17:47, Paul Sandoz wrote: : Ok, i removed it but added an assert for the array being non-null and containing at least one element. I also refined the documentation of the stream returning method in light of this: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8081678-enumeratio

Re: RFR(M, v13): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

On 06/03/2015 08:36 PM, Mandy Chung wrote: On 06/03/2015 08:29 AM, Dmitry Samersoff wrote: Updated webrev: http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13 I reviewed the jdk change. The version looks okay and some comments: ReferenceQueue.java - you should eliminate the use

Re: RFR 8072773 (fs) Files.lines needs a better splitting implementation for stream source

On 06/03/2015 08:53 AM, Paul Sandoz wrote: Hi, Please review an optimization for Files.lines for certain charsets: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8072773-File-lines/webrev/ If a charset is say US-ASCII or UTF-8 it is possible to implement an efficient splitting Spliterator th

Re: [8u-dev] Review request : JDK-8068416: LFGarbageCollectedTest.java fails with OOME: "GC overhead limit exceeded"

I bumped into this failure myself today. I think you've got a typo. 440 should be 40. Looks like a good approach otherwise. Regards, Sean. On 03/06/2015 17:33, Konstantin Shefov wrote: Hello, Please review the test bug fix https://bugs.openjdk.java.net/browse/JDK-8068416 Webrev is http://cr.

Re: RFR 8071597 Add Stream dropWhile and takeWhile operations

Hi Paul, This is a usefull addition to Stream API for sequential ordered streams. But does it have any utility in unordered streams at all? Wouldn't it be better to just throw an IllegalStateException or something if the stream is not ordered? I can't imagine currently a situation where I woul

Re: RFR: 8081824: Remove dead code GetPublicJREHome in the launcher

Looks good. Mandy On 06/03/2015 11:48 AM, Kumar Srinivasan wrote: Hello, Please review the removal of launcher/windows/registry dead code, the details of which are in the bug report. https://bugs.openjdk.java.net/browse/JDK-8081824 The webrev: http://cr.openjdk.java.net/~ksrini/8081824/webrev

RFR: 8081824: Remove dead code GetPublicJREHome in the launcher

Hello, Please review the removal of launcher/windows/registry dead code, the details of which are in the bug report. https://bugs.openjdk.java.net/browse/JDK-8081824 The webrev: http://cr.openjdk.java.net/~ksrini/8081824/webrev.00/ Thanks Kumar

Re: RFR 9: 8067808 : java/lang/ProcessBuilder/Basic.java failed on Assertion

Hi Ivan, Thanks, Roger There is a pending enhancement to ProcessHandle to deal with uniqueness of pids. I can look at adding a note to process.getPid also. (The pid will be the pid when the process was alive.) Roger On 6/3/2015 6:58 AM, Ivan Gerasimov wrote: Hi Roger! Seems Okay to me.

Re: RFR(M, v13): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

On 06/03/2015 08:29 AM, Dmitry Samersoff wrote: Updated webrev: http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13 I reviewed the jdk change. The version looks okay and some comments: ReferenceQueue.java - you should eliminate the use of rawtype: 84 Reference rn = r.

Re: RFR 8081678: Add Stream returning methods to classes where there currently exist only Enumeration returning methods

looks good Paul, thank you Best Lance On Jun 3, 2015, at 12:35 PM, Paul Sandoz wrote: > > On Jun 3, 2015, at 12:06 AM, Lance Andersen wrote: > >> Hi Paul, >> >> All the changes seem reasonable. A couple minor suggestions >> >> - DriverManager.drivers() - I do not think we need to repeat th

Re: RFR 9: 8081567 : java/lang/ProcessHandle/InfoTest.java failed Cannot run program "whoami"

Hi Ivan, Corrections updated in place: http://cr.openjdk.java.net/~rriggs/webrev-whoami-8081567/ On 6/3/2015 5:48 AM, Ivan Gerasimov wrote: On 03.06.2015 1:22, Roger Riggs wrote: Hi Ivan, Thanks for the review. Updated the webrev in place with 2 corrections. http://cr.openjdk.java

Re: RFR 8080640: Reduce copying when reading JAR/ZIP entries

Staffan, I'm not convinced that the benefit here is significant enough to change the getInputStream() to return a ByteArrayInputStream, given this can be easily achieved by wrapping the returned byte[] from getBytes(ZipEntry) at user's site. I would suggest to file a separate rfe on this disagree

Re: RFR: JDK-8079063: ZoneOffsetTransitionRule.of should throw IAE for non-zero nanoseconds

Hi Peter, Looks good; thanks for the extra checks. Roger On 6/3/2015 9:45 AM, Peter Levart wrote: Hi Roger, Now that CCC has approved the change in spec., I have prepared the final fix: http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/webrev.02/ I added asser

Re: RFR 8081678: Add Stream returning methods to classes where there currently exist only Enumeration returning methods

On Jun 3, 2015, at 12:46 PM, Alan Bateman wrote: > > On 02/06/2015 14:37, Paul Sandoz wrote: > >> : >> >> There is one small area of uncertainty with NetworkInterface. Can the >> following method ever return null? >> >> 342 public static Enumeration getNetworkInterfaces() >> 343

Re: Lower overhead String encoding/decoding

On 06/03/2015 07:34 PM, Xueming Shen wrote: > I'm still planning to be the sponsor for this RFE and get this one > into the jdk9. We are current working on JEP 254 [1][2][3]in which > the internal storage mechanism is changed from char[] to byte[]. If > this JEP get approved and is scheduled to ta

Re: RFR 8081678: Add Stream returning methods to classes where there currently exist only Enumeration returning methods

On Jun 3, 2015, at 12:06 AM, Lance Andersen wrote: > Hi Paul, > > All the changes seem reasonable. A couple minor suggestions > > - DriverManager.drivers() - I do not think we need to repeat the note from > getDrivers(), otherwise, I would use {@code} vs in the new javadoc > comment Remo

Re: Lower overhead String encoding/decoding

Richard, I'm still planning to be the sponsor for this RFE and get this one into the jdk9. We are current working on JEP 254 [1][2][3]in which the internal storage mechanism is changed from char[] to byte[]. If this JEP get approved and is scheduled to target JDK9, the decoding/encoding pieces

[8u-dev] Review request : JDK-8068416: LFGarbageCollectedTest.java fails with OOME: "GC overhead limit exceeded"

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

Re: RFR [9] 8072839: JAX-B Plugability Layer: using java.util.ServiceLoader

On 02/06/2015 13:32, Miroslav Kos wrote: Hi Alan, Daniel, would you have some time to check the changes in this one? The updated version looks okay to me. -Alan

Re: RFR 8071597 Add Stream dropWhile and takeWhile operations

Hi Stuart, I had prepared an alternative rendition stashed away just in case this came up :-) I still want to retain a punchy short first paragraph. What do you think about the following? Stream skip(long n); /** - * Returns a stream consisting of the longest prefix of elements

Re: RFR 8072773 (fs) Files.lines needs a better splitting implementation for stream source

On 03/06/2015 16:53, Paul Sandoz wrote: Hi, Please review an optimization for Files.lines for certain charsets: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8072773-File-lines/webrev/ If a charset is say US-ASCII or UTF-8 it is possible to implement an efficient splitting Spliterator that

Re: Lower overhead String encoding/decoding

Hi gents, My apology for the delay, things are little slow during this period of the >> year:-) >> I will sponsor the rfe and prepare for the CCC (internally). We may need >> go through >> the api and the implementation one more time. >> > > I was just wondering if there was any news on the CCC fo

Re: RFR 8071597 Add Stream dropWhile and takeWhile operations

On Jun 2, 2015, at 8:58 PM, Chris Hegarty wrote: > Very nice. I just looked over the spec, for now. > > * @param predicate a href="package-summary.html#NonInterference">non-interfering, > * href="package-summary.html#Statelessness">stateless > *

RFR 8072773 (fs) Files.lines needs a better splitting implementation for stream source

Hi, Please review an optimization for Files.lines for certain charsets: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8072773-File-lines/webrev/ If a charset is say US-ASCII or UTF-8 it is possible to implement an efficient splitting Spliterator that scans bytes from a mid-point to search for

Re: RFR(M, v13): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

Everyone, Updated webrev: http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13 Changes to oop.inline.hpp is reverted and find_field used directly is diagnostic command. -Dmitry On 2015-06-03 11:48, Dmitry Samersoff wrote: > Everyone, > > Updated webrev: > > http://cr.openjdk.java.net

RFR: 7130985: Four helper classes missing in Sun JDK

Meant to get this sorted a while back. There was a thread on this last year ( http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-July/027779.html ) A test has been added since then: http://cr.openjdk.java.net/~robm/7130985/webrev.corba/ http://cr.openjdk.java.net/~robm/7130985/webrev.j

Re: [9] RFR (XS) JDK-8081771: ProcessTool.createJavaProcessBuilder() needs new addTestVmAndJavaOptions argument

Hi Chris, The jdk testlibrary changes are fine. Roger On 6/2/2015 11:20 PM, Chris Plummer wrote: 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 Proce

Re: RFR: JDK-8079063: ZoneOffsetTransitionRule.of should throw IAE for non-zero nanoseconds

Hi Roger, Now that CCC has approved the change in spec., I have prepared the final fix: http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/webrev.02/ I added asserts to package-private constructors as a means to catch possible future mistakes as constructors are somet

Re: JDK 9 RFR of JDK-8081773: sun/net/www/protocol/https/ChunkedOutputStream.java references library that doesn't exist

Thank you Chris ! /Amy On 6/3/15 6:24 PM, Chris Hegarty wrote: Reviewed. I can push this for you. -Chris. On 03/06/15 11:11, Amy Lu wrote: sun/net/www/protocol/https/ChunkedOutputStream.java This test fails with jtreg4.1/b12 because jtreg4.1/b12 adds stricter checking of @library tags and t

Re: RFR 9: 8067808 : java/lang/ProcessBuilder/Basic.java failed on Assertion

Hi Roger! Seems Okay to me. Do you think it would make sense to make the doc for Process.getPid() clearer that it guarantees to return the same pid even after calling waitFor()? This's true for current implementation, but from the OS point of view, pid does not necessarily uniquely identifies

Re: RFR 8081678: Add Stream returning methods to classes where there currently exist only Enumeration returning methods

On 02/06/2015 14:37, Paul Sandoz wrote: : There is one small area of uncertainty with NetworkInterface. Can the following method ever return null? 342 public static Enumeration getNetworkInterfaces() 343 throws SocketException { 344 NetworkInterface[] netifs = getAl

Re: JDK 9 RFR of JDK-8081773: sun/net/www/protocol/https/ChunkedOutputStream.java references library that doesn't exist

Reviewed. I can push this for you. -Chris. On 03/06/15 11:11, Amy Lu wrote: sun/net/www/protocol/https/ChunkedOutputStream.java This test fails with jtreg4.1/b12 because jtreg4.1/b12 adds stricter checking of @library tags and the library directory defined in the test doesn't exist. Please he

JDK 9 RFR of JDK-8081773: sun/net/www/protocol/https/ChunkedOutputStream.java references library that doesn't exist

sun/net/www/protocol/https/ChunkedOutputStream.java This test fails with jtreg4.1/b12 because jtreg4.1/b12 adds stricter checking of @library tags and the library directory defined in the test doesn't exist. Please help to review and sponsor this fix, removed the unneeded @library. bug: http

Re: RFR 9: 8081567 : java/lang/ProcessHandle/InfoTest.java failed Cannot run program "whoami"

On 03.06.2015 1:22, Roger Riggs wrote: Hi Ivan, Thanks for the review. Updated the webrev in place with 2 corrections. http://cr.openjdk.java.net/~rriggs/webrev-whoami-8081567 On 6/2/2015 5:37 PM, Ivan Gerasimov wrote: Hi Roger! On 02.06.2015 0:38, Roger Riggs wrote: Please review a ch

Re: [8u-dev] Review request : JDK-8062904: TEST_BUG: Tests java/lang/invoke/LFCaching fail when run with -Xcomp option

Hi Vladimir On 02.06.2015 21:51, Vladimir Ivanov wrote: Konstantin, It seems we have only this bug that manifests the problem. As I understand, this is a product issue, not test. My question was about the symptoms - how the test can fail. If the test ignores NSME & VME exceptions, will it alw

Re: RFR(M, v11): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

On 2015-06-02 23:04, Dmitry Samersoff wrote: Mikael, The reason of placing get_filed_offset_by_name to the oop.inline is that hotspot widely duplicate this code. Symbol is used to identify a field within klass, not a klass them self so I think it's OK to have it tied to the oopDesc. Ok, I d

Re: RFR(M, v12): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

Everyone, Updated webrev: http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.12 getFinalizerHistogram and FinalizerHistogramEntry moved to separate package-private class. Hotspot code changed accordingly. getFinalizerHistogram updated to use Peter's code. -Dmitry On 2015-06-03 09:06,

Re: [9] RFR (M) 8054386: Allow Java debugging when CDS is enabled

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 fold

Re: JDK 9 RFR of JDK-8081775: two lib/testlibrary tests are failing with "Error. failed to clean up files after test" with jtreg 4.1 b12

Looks good! Thanks, /Staffan > On 3 jun 2015, at 05:31, Amy Lu wrote: > > lib/testlibrary/OutputAnalyzerReportingTest.java > lib/testlibrary/OutputAnalyzerTest.java > > These tests fail with jtreg4.1/b12 because jtreg4.1/b12 adds stricter > checking of @library tags and the library directory