Re: RFR (S) 8059677: Thread.getName() instantiates Strings
Thanks Staffan! -Aleksey. On 11.11.2014 23:38, Staffan Larsen wrote: SA changes look good. /Staffan On 11 nov 2014, at 15:40, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: Hi, On 11/09/2014 09:45 PM, Aleksey Shipilev wrote: Thread.getName() returns String, and does new String instantiation every time, because the thread name is stored in char[]. Even though we use a private String constructor that shares the char[] array without copying it, this still hurts some use cases (think extra-fast logging). To the extent some people actually maintain MapThread, String to avoid it. https://bugs.openjdk.java.net/browse/JDK-8059677 Here's the attempt to maintain String instead of char[]: http://cr.openjdk.java.net/~shade/8059677/webrev.01.jdk/ http://cr.openjdk.java.net/~shade/8059677/webrev.01.hs/ Updated webrevs: http://cr.openjdk.java.net/~shade/8059677/webrev.02.jdk/ http://cr.openjdk.java.net/~shade/8059677/webrev.02.hs/ This version incorporates feedbacks from Chris, Staffan and David. I think it is very close to what we would like to push. Opinions? Testing: JPRT, jdk/test/java/lang/Thread jtreg, hotspot/test/runtime/ jtreg, vm.quick.testlist, nsk.jvmti.testlist, svc.quick.testlist, vm.tmtools.testlist Thanks, -Aleksey.
Re: RFR (S) 8059677: Thread.getName() instantiates Strings
Hi David,
On 12.11.2014 07:48, David Holmes wrote:
On 12/11/2014 12:40 AM, Aleksey Shipilev wrote:
All looks good to me.
Thanks for the review!
But I also noticed this strange (to me) assertion in javaClasses.cpp
void java_lang_Thread::set_name(oop java_thread, oop name) {
assert(java_thread-obj_field(_name_offset) == NULL, name should be
NULL);
java_thread-obj_field_put(_name_offset, name);
}
and on investigation it seems like this is dead code - I couldn't locate
a call to java_lang_Thread::set_name ?? It would only be usable on an
attaching thread (else name can't be null) and we pass the name to the
Thread constructor in that case.
set_name is not used, as I mentioned earlier -- that makes the change
even more safe. I was even tempted to drop the setter completely, but
it would break the symmetry against other setters and getters. I dropped
the assert at set_name in this update:
http://cr.openjdk.java.net/~shade/8059677/webrev.03.hs/
http://cr.openjdk.java.net/~shade/8059677/webrev.03.jdk/
The only difference against the previous version is the dropped assert,
so I haven't re-spinned the tests.
Thanks,
-Aleksey.
Re: Review request: JDK-8055723 Replace concat String to append in StringBuilder parameters
Hi Otávio,
I now think you could replace
if (!expected.isEmpty())
with
assert !expected.isEmpty();
If expected ever would be empty, the only thing which happens is, that a ' is missing in a message
which anyway doesn't make sense without arguments.
-Ulf
Am 12.11.2014 um 08:19 schrieb Otávio Gonçalves de Santana:
Thank you Ulf.
http://cr.openjdk.java.net/~weijun/8055723/webrev.01/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.01/
On Sat, Nov 8, 2014 at 3:46 PM, Ulf Zibis ulf.zi...@cosoco.de
mailto:ulf.zi...@cosoco.de wrote:
Hi Otávio,
in sun/tools/jstat/SyntaxException.java I see a possible enhencement (maybe
applies to other
places too):
65 public SyntaxException(int lineno, SetString expected, Token
found) {
66 StringBuilder msg = new StringBuilder(A + B * expected.size());
67
68 msg.append(Syntax error at line ).append(lineno).append(:
Expected one of \');
69
71 for (String keyWord : expected) {
72 msg.append(keyWord).append('|');
73 }
74 // if (!expected.isEmpty()) // only needed if expected may be
empty.
75 msg.setLength(msg.length() - 1);
76
81 message = msg.append(\', Found
).append(found.toMessage()).toString();
83 }
* Additionally at many places you could similarly introduce the foreach
syntax.
-Ulf
Am 02.11.2014 um 15:45 schrieb Otávio Gonçalves de Santana:
Could another reviewer look these codes, please.
http://cr.openjdk.java.net/~weijun/8055723/webrev.00/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.00/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.00/
On Fri, Oct 24, 2014 at 3:25 AM, Otávio Gonçalves de Santana
otavioj...@java.net
mailto:otavioj...@java.net mailto:otavioj...@java.net
mailto:otavioj...@java.net wrote:
Thank you Ulf.
I removed the fix in toString method and in debug classes:
http://cr.openjdk.java.net/~weijun/8055723/webrev.00/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.00/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.00/
On Mon, Oct 20, 2014 at 10:26 PM, Ulf Zibis ulf.zi...@cosoco.de
mailto:ulf.zi...@cosoco.de mailto:ulf.zi...@cosoco.de
mailto:ulf.zi...@cosoco.de
wrote:
Am 21.10.2014 um 01:02 schrieb Otávio Gonçalves de Santana:
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8055723
WEBREV:
http://cr.openjdk.java.net/~weijun/8055723/client/webrev.02/
http://cr.openjdk.java.net/%7Eweijun/8055723/client/webrev.02/
http://cr.openjdk.java.net/%7Eweijun/8055723/client/webrev.02/
WEBREV:
http://cr.openjdk.java.net/~weijun/8055723/core/webrev.03/
http://cr.openjdk.java.net/%7Eweijun/8055723/core/webrev.03/
http://cr.openjdk.java.net/%7Eweijun/8055723/core/webrev.03/
I did not look through all sources.
In Scanner.java I discovered:
1307 sb.append([delimiters=).append(delimPattern).append(']');
1308 sb.append([position=).append(position).append(']');
...
Maybe better:
1307 sb.append([delimiters=).append(delimPattern);
1308 sb.append(][position=).append(position);
...
-Ulf
-- Otávio Gonçalves de Santana
blog: http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
site: _http://about.me/otaviojava_
55 (11) 98255-3513 tel:55%20%2811%29%2098255-3513
tel:55%20%2811%29%2098255-3513
--
Otávio Gonçalves de Santana
blog: http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
site: _http://about.me/otaviojava_
55 (11) 98255-3513 tel:55%20%2811%29%2098255-3513
tel:55%20%2811%29%2098255-3513
--
Otávio Gonçalves de Santana
blog: http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
site: _http://about.me/otaviojava_
55 (11) 98255-3513
Re: [9] RFR(L) 8013267 : move MemberNameTable from native code to Java heap, use to intern MemberNames
Hello Peter, I was looking at this (thinking it would be a useful thing to benchmark, looking for possible improvements) and noticed that you rely on the hashed objects having a sensible value-dependent hashcode (as opposed to the default Object hashcode). Sadly, this seems not to be the case for MemberNames or for “Types”. I am sorely tempted to repair this glitch, not sure if it fits in the scope of the original bug, but there’s a lot to be said for future-performance-proofing. David On 2014-11-09, at 7:55 AM, Peter Levart peter.lev...@gmail.com wrote: Hi David, I played a little with the idea of having a hash table instead of packed sorted array for interning. Using ConcurrentHashMap would present quite some memory overhead. A more compact representation is possible in the form of a linear-scan hash table where elements of array are MemberNames themselves: http://cr.openjdk.java.net/~plevart/misc/MemberName.intern/jdk.06.diff/ This is a drop-in replacement for MemberName on top of your jdk.06 patch. If you have some time, you can run this with your performance tests to see if it presents any difference. If not, then perhaps this interning is not so performance critical after all. Regards, Peter
Re: [9] RFR(L) 8013267 : move MemberNameTable from native code to Java heap, use to intern MemberNames
Hello Peter, Sadly, this seems not to be the case for MemberNames or for “Types”. That statement is inoperative. Mistakes were made. It’s compareTo that they lack. David On 2014-11-09, at 7:55 AM, Peter Levart peter.lev...@gmail.com wrote: Hi David, I played a little with the idea of having a hash table instead of packed sorted array for interning. Using ConcurrentHashMap would present quite some memory overhead. A more compact representation is possible in the form of a linear-scan hash table where elements of array are MemberNames themselves: http://cr.openjdk.java.net/~plevart/misc/MemberName.intern/jdk.06.diff/ This is a drop-in replacement for MemberName on top of your jdk.06 patch. If you have some time, you can run this with your performance tests to see if it presents any difference. If not, then perhaps this interning is not so performance critical after all. Regards, Peter
RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed
Please review test changes to ProcessBuilder Basic.java to add some debugging information. The test has been failing intermittently. The wait times have been extended to see if the systems are just slow. The failure criteria have not changed. Suggestions welcome. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/ issue: 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed Thanks, Roger [1] https://bugs.openjdk.java.net/browse/JDK-8043477
Re: RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed
The print statement below seems redundant with the assertion failure message.
You could improve the assertion message instead if need be.
Adding thousand separator underscores to 2L would help a little.
I like my little helper
static long millisElapsedSince(long startNanoTime) {
return NANOSECONDS.toMillis(System.nanoTime() - startNanoTime);
}
+System.out.printf( waitFor process: delta: %d%n,(end - start) );
+
if ((end - start) 2L * (AIX.is() ? 2 : 1))
fail(Test failed: waitFor took too long ( + (end -
start) + ns));
200 ms timeout for subprocesses to finish is just too damn low. In
j.u.c. tests we switched to 10 seconds for most long timeouts
(LONG_DELAY_MS) and are happy with the disappearance of rare flaky
results.
On Wed, Nov 12, 2014 at 11:34 AM, roger riggs roger.ri...@oracle.com wrote:
Please review test changes to ProcessBuilder Basic.java to add some
debugging information.
The test has been failing intermittently. The wait times have been extended
to see
if the systems are just slow. The failure criteria have not changed.
Suggestions welcome.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
issue:
8043477: java/lang/ProcessBuilder/Basic.java failed with:
java.lang.AssertionError: Some tests failed
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8043477
Re: RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed
Eesh, Sorry Roger, I have something like this on my todo.
Martin, my concern with the long delay approach is that it effectively
nullifies the point of the test. Given that this test has been flakey
the approach has been to simply bump the acceptable delta by another
100ms or so every time. Since we've had to do this repeatedly however,
I'm beginning to question whether the test is more trouble than its worth.
-Rob
On 12/11/14 19:44, Martin Buchholz wrote:
The print statement below seems redundant with the assertion failure message.
You could improve the assertion message instead if need be.
Adding thousand separator underscores to 2L would help a little.
I like my little helper
static long millisElapsedSince(long startNanoTime) {
return NANOSECONDS.toMillis(System.nanoTime() - startNanoTime);
}
+System.out.printf( waitFor process: delta: %d%n,(end - start) );
+
if ((end - start) 2L * (AIX.is() ? 2 : 1))
fail(Test failed: waitFor took too long ( + (end -
start) + ns));
200 ms timeout for subprocesses to finish is just too damn low. In
j.u.c. tests we switched to 10 seconds for most long timeouts
(LONG_DELAY_MS) and are happy with the disappearance of rare flaky
results.
On Wed, Nov 12, 2014 at 11:34 AM, roger riggs roger.ri...@oracle.com wrote:
Please review test changes to ProcessBuilder Basic.java to add some
debugging information.
The test has been failing intermittently. The wait times have been extended
to see
if the systems are just slow. The failure criteria have not changed.
Suggestions welcome.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
issue:
8043477: java/lang/ProcessBuilder/Basic.java failed with:
java.lang.AssertionError: Some tests failed
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8043477
Re: RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed
Hi Martin, Rob,
I thought the point of the test was to verify the timeout logic in
waitFor(timeout).
Adding the prints without changing the test criteria was intended to
gather data
about the distribution.
A long timeout would still catch a failure in the reaper/waitFor
communication
so maybe that's ok.
As for printing the time, I would switch to the newer
java.time.Instant/Duration
which have a decent toString.
Roger
On 11/12/2014 3:02 PM, Rob McKenna wrote:
Eesh, Sorry Roger, I have something like this on my todo.
Martin, my concern with the long delay approach is that it effectively
nullifies the point of the test. Given that this test has been flakey
the approach has been to simply bump the acceptable delta by another
100ms or so every time. Since we've had to do this repeatedly however,
I'm beginning to question whether the test is more trouble than its
worth.
-Rob
On 12/11/14 19:44, Martin Buchholz wrote:
The print statement below seems redundant with the assertion failure
message.
You could improve the assertion message instead if need be.
Adding thousand separator underscores to 2L would help a little.
I like my little helper
static long millisElapsedSince(long startNanoTime) {
return NANOSECONDS.toMillis(System.nanoTime() - startNanoTime);
}
+System.out.printf( waitFor process: delta: %d%n,(end -
start) );
+
if ((end - start) 2L * (AIX.is() ? 2 : 1))
fail(Test failed: waitFor took too long ( + (end -
start) + ns));
200 ms timeout for subprocesses to finish is just too damn low. In
j.u.c. tests we switched to 10 seconds for most long timeouts
(LONG_DELAY_MS) and are happy with the disappearance of rare flaky
results.
On Wed, Nov 12, 2014 at 11:34 AM, roger riggs
roger.ri...@oracle.com wrote:
Please review test changes to ProcessBuilder Basic.java to add some
debugging information.
The test has been failing intermittently. The wait times have been
extended
to see
if the systems are just slow. The failure criteria have not changed.
Suggestions welcome.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
issue:
8043477: java/lang/ProcessBuilder/Basic.java failed with:
java.lang.AssertionError: Some tests failed
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8043477
Re: RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed
Looking over the test again (I was the original author IIRC),
I think this test still has lots of value with a longer timeout.
Experience seems to show that 200ms is not a long enough timeout for
system operations to use in non-flaky tests, including everything in
Process handling.
I say we ncrease the timeout by at least 10x and move on.
On Wed, Nov 12, 2014 at 12:16 PM, roger riggs roger.ri...@oracle.com wrote:
Hi Martin, Rob,
I thought the point of the test was to verify the timeout logic in
waitFor(timeout).
Adding the prints without changing the test criteria was intended to gather
data
about the distribution.
A long timeout would still catch a failure in the reaper/waitFor
communication
so maybe that's ok.
As for printing the time, I would switch to the newer
java.time.Instant/Duration
which have a decent toString.
Roger
On 11/12/2014 3:02 PM, Rob McKenna wrote:
Eesh, Sorry Roger, I have something like this on my todo.
Martin, my concern with the long delay approach is that it effectively
nullifies the point of the test. Given that this test has been flakey the
approach has been to simply bump the acceptable delta by another 100ms or so
every time. Since we've had to do this repeatedly however, I'm beginning to
question whether the test is more trouble than its worth.
-Rob
On 12/11/14 19:44, Martin Buchholz wrote:
The print statement below seems redundant with the assertion failure
message.
You could improve the assertion message instead if need be.
Adding thousand separator underscores to 2L would help a little.
I like my little helper
static long millisElapsedSince(long startNanoTime) {
return NANOSECONDS.toMillis(System.nanoTime() - startNanoTime);
}
+System.out.printf( waitFor process: delta: %d%n,(end -
start) );
+
if ((end - start) 2L * (AIX.is() ? 2 : 1))
fail(Test failed: waitFor took too long ( + (end -
start) + ns));
200 ms timeout for subprocesses to finish is just too damn low. In
j.u.c. tests we switched to 10 seconds for most long timeouts
(LONG_DELAY_MS) and are happy with the disappearance of rare flaky
results.
On Wed, Nov 12, 2014 at 11:34 AM, roger riggs roger.ri...@oracle.com
wrote:
Please review test changes to ProcessBuilder Basic.java to add some
debugging information.
The test has been failing intermittently. The wait times have been
extended
to see
if the systems are just slow. The failure criteria have not changed.
Suggestions welcome.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
issue:
8043477: java/lang/ProcessBuilder/Basic.java failed with:
java.lang.AssertionError: Some tests failed
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8043477
Re: RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed
Hi Martin,
I updated the webrev to use long timeouts for the test in question; they
can't be too long
because the spawned sleep is only alive for 10 seconds.
Take a look please:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
Thanks, Roger
On 11/12/2014 4:45 PM, Martin Buchholz wrote:
Looking over the test again (I was the original author IIRC),
I think this test still has lots of value with a longer timeout.
Experience seems to show that 200ms is not a long enough timeout for
system operations to use in non-flaky tests, including everything in
Process handling.
I say we ncrease the timeout by at least 10x and move on.
On Wed, Nov 12, 2014 at 12:16 PM, roger riggs roger.ri...@oracle.com wrote:
Hi Martin, Rob,
I thought the point of the test was to verify the timeout logic in
waitFor(timeout).
Adding the prints without changing the test criteria was intended to gather
data
about the distribution.
A long timeout would still catch a failure in the reaper/waitFor
communication
so maybe that's ok.
As for printing the time, I would switch to the newer
java.time.Instant/Duration
which have a decent toString.
Roger
On 11/12/2014 3:02 PM, Rob McKenna wrote:
Eesh, Sorry Roger, I have something like this on my todo.
Martin, my concern with the long delay approach is that it effectively
nullifies the point of the test. Given that this test has been flakey the
approach has been to simply bump the acceptable delta by another 100ms or so
every time. Since we've had to do this repeatedly however, I'm beginning to
question whether the test is more trouble than its worth.
-Rob
On 12/11/14 19:44, Martin Buchholz wrote:
The print statement below seems redundant with the assertion failure
message.
You could improve the assertion message instead if need be.
Adding thousand separator underscores to 2L would help a little.
I like my little helper
static long millisElapsedSince(long startNanoTime) {
return NANOSECONDS.toMillis(System.nanoTime() - startNanoTime);
}
+System.out.printf( waitFor process: delta: %d%n,(end -
start) );
+
if ((end - start) 2L * (AIX.is() ? 2 : 1))
fail(Test failed: waitFor took too long ( + (end -
start) + ns));
200 ms timeout for subprocesses to finish is just too damn low. In
j.u.c. tests we switched to 10 seconds for most long timeouts
(LONG_DELAY_MS) and are happy with the disappearance of rare flaky
results.
On Wed, Nov 12, 2014 at 11:34 AM, roger riggs roger.ri...@oracle.com
wrote:
Please review test changes to ProcessBuilder Basic.java to add some
debugging information.
The test has been failing intermittently. The wait times have been
extended
to see
if the systems are just slow. The failure criteria have not changed.
Suggestions welcome.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
issue:
8043477: java/lang/ProcessBuilder/Basic.java failed with:
java.lang.AssertionError: Some tests failed
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8043477
Re: RFR (S) 8059677: Thread.getName() instantiates Strings
On 12/11/2014 8:23 PM, Aleksey Shipilev wrote:
Hi David,
On 12.11.2014 07:48, David Holmes wrote:
On 12/11/2014 12:40 AM, Aleksey Shipilev wrote:
All looks good to me.
Thanks for the review!
But I also noticed this strange (to me) assertion in javaClasses.cpp
void java_lang_Thread::set_name(oop java_thread, oop name) {
assert(java_thread-obj_field(_name_offset) == NULL, name should be
NULL);
java_thread-obj_field_put(_name_offset, name);
}
and on investigation it seems like this is dead code - I couldn't locate
a call to java_lang_Thread::set_name ?? It would only be usable on an
attaching thread (else name can't be null) and we pass the name to the
Thread constructor in that case.
set_name is not used, as I mentioned earlier -- that makes the change
Sorry, I missed that comment.
even more safe. I was even tempted to drop the setter completely, but
it would break the symmetry against other setters and getters. I dropped
the assert at set_name in this update:
http://cr.openjdk.java.net/~shade/8059677/webrev.03.hs/
http://cr.openjdk.java.net/~shade/8059677/webrev.03.jdk/
The only difference against the previous version is the dropped assert,
so I haven't re-spinned the tests.
OK. I'm more inclined to delete unused code but it is fine as is.
Thanks,
David
Thanks,
-Aleksey.
Re: RFR (S) 8059677: Thread.getName() instantiates Strings
On 11.11.2014 17:40, Aleksey Shipilev wrote: On 11/09/2014 09:45 PM, Aleksey Shipilev wrote: Thread.getName() returns String, and does new String instantiation every time, because the thread name is stored in char[]. Even though we use a private String constructor that shares the char[] array without copying it, this still hurts some use cases (think extra-fast logging). To the extent some people actually maintain MapThread, String to avoid it. https://bugs.openjdk.java.net/browse/JDK-8059677 Here's the attempt to maintain String instead of char[]: http://cr.openjdk.java.net/~shade/8059677/webrev.01.jdk/ http://cr.openjdk.java.net/~shade/8059677/webrev.01.hs/ Updated webrevs: http://cr.openjdk.java.net/~shade/8059677/webrev.02.jdk/ http://cr.openjdk.java.net/~shade/8059677/webrev.02.hs/ All right, third time a charm. All reviewers seem to be happy with these changes: http://cr.openjdk.java.net/~shade/8059677/webrev.03.jdk/ http://cr.openjdk.java.net/~shade/8059677/webrev.03.hs/ Coleen had volunteered to sponsor them (thanks!), here are the changesets: http://cr.openjdk.java.net/~shade/8059677/8059677-jdk.changeset http://cr.openjdk.java.net/~shade/8059677/8059677-hs.changeset Thanks, -Aleksey.
Re: RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed
Hi Roger,
On Wed, Nov 12, 2014 at 1:57 PM, roger riggs roger.ri...@oracle.com wrote:
Hi Martin,
I updated the webrev to use long timeouts for the test in question; they
can't be too long
because the spawned sleep is only alive for 10 seconds.
I am looking at the sleep command invocation, but it's an indefinite
sleep. I don't see 10 seconds anywhere.
Take a look please:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
2313 Integer exitValue = null;
2314 try {
2315 exitValue = p.exitValue();
2316 } catch (IllegalStateException ise) {
2317 // no exitValue
2318 }
2319 if ((end - start) 6 * 1_000_000_000)
2320 fail(Test failed: waitFor took too long on a
dead process. ( + (end - start) + ns)
2321 + , exitValue: + Objects.requireNonNull(exitValue));
exitValue throws IllegalThreadStateException, not IllegalStateException.
I would fail separately in case of ITSE. It looks instead like you'll
fail with uninformative NPE.
2301 p.waitFor(1000, TimeUnit.MILLISECONDS);
2304 if ((end - start) 500_000_000)
I am surprised the test is not testing that the initial timed waitFor
doesn't wait at least the specified time, instead of only half. I
consider returning from waitFor early a bug.
Thanks, Roger
On 11/12/2014 4:45 PM, Martin Buchholz wrote:
Looking over the test again (I was the original author IIRC),
I think this test still has lots of value with a longer timeout.
Experience seems to show that 200ms is not a long enough timeout for
system operations to use in non-flaky tests, including everything in
Process handling.
I say we ncrease the timeout by at least 10x and move on.
On Wed, Nov 12, 2014 at 12:16 PM, roger riggs roger.ri...@oracle.com
wrote:
Hi Martin, Rob,
I thought the point of the test was to verify the timeout logic in
waitFor(timeout).
Adding the prints without changing the test criteria was intended to
gather
data
about the distribution.
A long timeout would still catch a failure in the reaper/waitFor
communication
so maybe that's ok.
As for printing the time, I would switch to the newer
java.time.Instant/Duration
which have a decent toString.
Roger
On 11/12/2014 3:02 PM, Rob McKenna wrote:
Eesh, Sorry Roger, I have something like this on my todo.
Martin, my concern with the long delay approach is that it effectively
nullifies the point of the test. Given that this test has been flakey
the
approach has been to simply bump the acceptable delta by another 100ms
or so
every time. Since we've had to do this repeatedly however, I'm beginning
to
question whether the test is more trouble than its worth.
-Rob
On 12/11/14 19:44, Martin Buchholz wrote:
The print statement below seems redundant with the assertion failure
message.
You could improve the assertion message instead if need be.
Adding thousand separator underscores to 2L would help a
little.
I like my little helper
static long millisElapsedSince(long startNanoTime) {
return NANOSECONDS.toMillis(System.nanoTime() -
startNanoTime);
}
+System.out.printf( waitFor process: delta: %d%n,(end -
start) );
+
if ((end - start) 2L * (AIX.is() ? 2 : 1))
fail(Test failed: waitFor took too long ( + (end -
start) + ns));
200 ms timeout for subprocesses to finish is just too damn low. In
j.u.c. tests we switched to 10 seconds for most long timeouts
(LONG_DELAY_MS) and are happy with the disappearance of rare flaky
results.
On Wed, Nov 12, 2014 at 11:34 AM, roger riggs roger.ri...@oracle.com
wrote:
Please review test changes to ProcessBuilder Basic.java to add some
debugging information.
The test has been failing intermittently. The wait times have been
extended
to see
if the systems are just slow. The failure criteria have not changed.
Suggestions welcome.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
issue:
8043477: java/lang/ProcessBuilder/Basic.java failed with:
java.lang.AssertionError: Some tests failed
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8043477
Re: RFR: AARCH64: 8064594: Top-level JDK changes
Oh. I just replied to the wrong email. Anyway, here it goes again: Maybe we should CC sound-dev (if that’s the correct list)? The new jvm.cfg files should only have a copyright year of 2014. Otherwise this looks good. On Nov 12, 2014, at 3:48 AM, Andrew Haley a...@redhat.com wrote: The changes for the /jdk subdirectory. The missing files problem bit me again. New webrev: http://cr.openjdk.java.net/~aph/aarch64-JDK-8064594-1/ Apologies, Andrew.
Re: Review request: JDK-8055723 Replace concat String to append in StringBuilder parameters
But this class is an Exception, doesn't make sense an exception get another
Exception.
IMHO: I prefer this way
On Wed, Nov 12, 2014 at 8:36 AM, Ulf Zibis ulf.zi...@cosoco.de wrote:
Hi Otávio,
I now think you could replace
if (!expected.isEmpty())
with
assert !expected.isEmpty();
If expected ever would be empty, the only thing which happens is, that a
' is missing in a message which anyway doesn't make sense without
arguments.
-Ulf
Am 12.11.2014 um 08:19 schrieb Otávio Gonçalves de Santana:
Thank you Ulf.
http://cr.openjdk.java.net/~weijun/8055723/webrev.01/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.01/
On Sat, Nov 8, 2014 at 3:46 PM, Ulf Zibis ulf.zi...@cosoco.de mailto:
ulf.zi...@cosoco.de wrote:
Hi Otávio,
in sun/tools/jstat/SyntaxException.java I see a possible enhencement
(maybe applies to other
places too):
65 public SyntaxException(int lineno, SetString expected,
Token found) {
66 StringBuilder msg = new StringBuilder(A + B *
expected.size());
67
68 msg.append(Syntax error at line
).append(lineno).append(: Expected one of \');
69
71 for (String keyWord : expected) {
72 msg.append(keyWord).append('|');
73 }
74 // if (!expected.isEmpty()) // only needed if expected
may be empty.
75 msg.setLength(msg.length() - 1);
76
81 message = msg.append(\', Found
).append(found.toMessage()).toString();
83 }
* Additionally at many places you could similarly introduce the
foreach syntax.
-Ulf
Am 02.11.2014 um 15:45 schrieb Otávio Gonçalves de Santana:
Could another reviewer look these codes, please.
http://cr.openjdk.java.net/~weijun/8055723/webrev.00/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.00/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.00/
On Fri, Oct 24, 2014 at 3:25 AM, Otávio Gonçalves de Santana
otavioj...@java.net
mailto:otavioj...@java.net mailto:otavioj...@java.net mailto:
otavioj...@java.net wrote:
Thank you Ulf.
I removed the fix in toString method and in debug classes:
http://cr.openjdk.java.net/~weijun/8055723/webrev.00/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.00/
http://cr.openjdk.java.net/%7Eweijun/8055723/webrev.00/
On Mon, Oct 20, 2014 at 10:26 PM, Ulf Zibis
ulf.zi...@cosoco.de
mailto:ulf.zi...@cosoco.de mailto:ulf.zi...@cosoco.de mailto:
ulf.zi...@cosoco.de
wrote:
Am 21.10.2014 um 01:02 schrieb Otávio Gonçalves de
Santana:
BUGURL: https://bugs.openjdk.java.net/
browse/JDK-8055723
WEBREV: http://cr.openjdk.java.net/~
weijun/8055723/client/webrev.02/
http://cr.openjdk.java.net/%7Eweijun/8055723/client/webrev.02/
http://cr.openjdk.java.net/%7Eweijun/8055723/client/
webrev.02/
WEBREV: http://cr.openjdk.java.net/~
weijun/8055723/core/webrev.03/
http://cr.openjdk.java.net/%7Eweijun/8055723/core/webrev.03/
http://cr.openjdk.java.net/%
7Eweijun/8055723/core/webrev.03/
I did not look through all sources.
In Scanner.java I discovered:
1307 sb.append([delimiters=).
append(delimPattern).append(']');
1308 sb.append([position=).
append(position).append(']');
...
Maybe better:
1307 sb.append([delimiters=).append(delimPattern);
1308 sb.append(][position=).append(position);
...
-Ulf
-- Otávio Gonçalves de Santana
blog: http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
site: _http://about.me/otaviojava_
55 (11) 98255-3513 tel:55%20%2811%29%2098255-3513
tel:55%20%2811%29%2098255-3513
-- Otávio Gonçalves de Santana
blog: http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
site: _http://about.me/otaviojava_
55 (11) 98255-3513 tel:55%20%2811%29%2098255-3513
tel:55%20%2811%29%2098255-3513
--
Otávio Gonçalves de Santana
blog: http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
site: _http://about.me/otaviojava_
55 (11) 98255-3513
--
Otávio Gonçalves de Santana
blog: http://otaviosantana.blogspot.com.br/
twitter: http://twitter.com/otaviojava
site: *http://about.me/otaviojava http://about.me/otaviojava*
55 (11) 98255-3513
Re: Review request: JDK-8055723 Replace concat String to append in StringBuilder parameters
I hope we can restrict the code change to what the bug description is about. IMHO this bug should only include cleanup and introduce no obvious behavior change. Any other fix can go to another bug. --Max On Nov 13, 2014, at 08:57, Otávio Gonçalves de Santana otavioj...@java.net wrote: But this class is an Exception, doesn't make sense an exception get another Exception. IMHO: I prefer this way On Wed, Nov 12, 2014 at 8:36 AM, Ulf Zibis ulf.zi...@cosoco.de wrote: Hi Otávio, I now think you could replace if (!expected.isEmpty()) with assert !expected.isEmpty(); If expected ever would be empty, the only thing which happens is, that a ' is missing in a message which anyway doesn't make sense without arguments. -Ulf ..
Re: Review request: JDK-8055723 Replace concat String to append in StringBuilder parameters
Ok, you're right. On Wed, Nov 12, 2014 at 11:19 PM, Wang Weijun weijun.w...@oracle.com wrote: I hope we can restrict the code change to what the bug description is about. IMHO this bug should only include cleanup and introduce no obvious behavior change. Any other fix can go to another bug. --Max On Nov 13, 2014, at 08:57, Otávio Gonçalves de Santana otavioj...@java.net wrote: But this class is an Exception, doesn't make sense an exception get another Exception. IMHO: I prefer this way On Wed, Nov 12, 2014 at 8:36 AM, Ulf Zibis ulf.zi...@cosoco.de wrote: Hi Otávio, I now think you could replace if (!expected.isEmpty()) with assert !expected.isEmpty(); If expected ever would be empty, the only thing which happens is, that a ' is missing in a message which anyway doesn't make sense without arguments. -Ulf .. -- Otávio Gonçalves de Santana blog: http://otaviosantana.blogspot.com.br/ twitter: http://twitter.com/otaviojava site: *http://about.me/otaviojava http://about.me/otaviojava* 55 (11) 98255-3513
Re: RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed
Hi Martin,
I think that sleep is the version that spawns a JavaChild process and
has its own comment interpreter. See line 309: Am I getting the sleeps
mixed up?
Roger
On 11/12/2014 6:32 PM, Martin Buchholz wrote:
Hi Roger,
On Wed, Nov 12, 2014 at 1:57 PM, roger riggs roger.ri...@oracle.com wrote:
Hi Martin,
I updated the webrev to use long timeouts for the test in question; they
can't be too long
because the spawned sleep is only alive for 10 seconds.
I am looking at the sleep command invocation, but it's an indefinite
sleep. I don't see 10 seconds anywhere.
Take a look please:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
2313 Integer exitValue = null;
2314 try {
2315 exitValue = p.exitValue();
2316 } catch (IllegalStateException ise) {
2317 // no exitValue
2318 }
2319 if ((end - start) 6 * 1_000_000_000)
2320 fail(Test failed: waitFor took too long on a
dead process. ( + (end - start) + ns)
2321 + , exitValue: + Objects.requireNonNull(exitValue));
exitValue throws IllegalThreadStateException, not IllegalStateException.
I would fail separately in case of ITSE. It looks instead like you'll
fail with uninformative NPE.
2301 p.waitFor(1000, TimeUnit.MILLISECONDS);
2304 if ((end - start) 500_000_000)
I am surprised the test is not testing that the initial timed waitFor
doesn't wait at least the specified time, instead of only half. I
consider returning from waitFor early a bug.
Thanks, Roger
On 11/12/2014 4:45 PM, Martin Buchholz wrote:
Looking over the test again (I was the original author IIRC),
I think this test still has lots of value with a longer timeout.
Experience seems to show that 200ms is not a long enough timeout for
system operations to use in non-flaky tests, including everything in
Process handling.
I say we ncrease the timeout by at least 10x and move on.
On Wed, Nov 12, 2014 at 12:16 PM, roger riggs roger.ri...@oracle.com
wrote:
Hi Martin, Rob,
I thought the point of the test was to verify the timeout logic in
waitFor(timeout).
Adding the prints without changing the test criteria was intended to
gather
data
about the distribution.
A long timeout would still catch a failure in the reaper/waitFor
communication
so maybe that's ok.
As for printing the time, I would switch to the newer
java.time.Instant/Duration
which have a decent toString.
Roger
On 11/12/2014 3:02 PM, Rob McKenna wrote:
Eesh, Sorry Roger, I have something like this on my todo.
Martin, my concern with the long delay approach is that it effectively
nullifies the point of the test. Given that this test has been flakey
the
approach has been to simply bump the acceptable delta by another 100ms
or so
every time. Since we've had to do this repeatedly however, I'm beginning
to
question whether the test is more trouble than its worth.
-Rob
On 12/11/14 19:44, Martin Buchholz wrote:
The print statement below seems redundant with the assertion failure
message.
You could improve the assertion message instead if need be.
Adding thousand separator underscores to 2L would help a
little.
I like my little helper
static long millisElapsedSince(long startNanoTime) {
return NANOSECONDS.toMillis(System.nanoTime() -
startNanoTime);
}
+System.out.printf( waitFor process: delta: %d%n,(end -
start) );
+
if ((end - start) 2L * (AIX.is() ? 2 : 1))
fail(Test failed: waitFor took too long ( + (end -
start) + ns));
200 ms timeout for subprocesses to finish is just too damn low. In
j.u.c. tests we switched to 10 seconds for most long timeouts
(LONG_DELAY_MS) and are happy with the disappearance of rare flaky
results.
On Wed, Nov 12, 2014 at 11:34 AM, roger riggs roger.ri...@oracle.com
wrote:
Please review test changes to ProcessBuilder Basic.java to add some
debugging information.
The test has been failing intermittently. The wait times have been
extended
to see
if the systems are just slow. The failure criteria have not changed.
Suggestions welcome.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/
issue:
8043477: java/lang/ProcessBuilder/Basic.java failed with:
java.lang.AssertionError: Some tests failed
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8043477
