Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
Hi Brian, This generally looks good, rather clever trick, i prefer it to using a cache. I agree with Joe some comments are required as it is not immediately obvious how this works. The additional tests seem adequate (overflow of the overflow), it's easy to go overboard e.g. you could test: BigInteger b = quotient.multiply(BigInteger.valueOf(radix + N)); for some range of N according to the radix under test. However, it might be useful to accurately test boundary conditions. Paul. On Jan 3, 2014, at 8:00 PM, Brian Burkhalter brian.burkhal...@oracle.com wrote: Issue:https://bugs.openjdk.java.net/browse/JDK-8030814 webrev: http://cr.openjdk.java.net/~bpb/8030814/webrev.2/ This review request follows from the discussion of last month in this thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024031.html The contributed patch before my minor tweaking of it is here http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024110.html with a detailed explanation of its logic here http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-December/024136.html I added to the java/lang/Long/Unsigned JTREG test the case from the issue report as well as some other cases which exercise both sides of the A v B overflow test. Thanks, Brian
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Can someone else give a second review on this.
Thank you
Tristan
On 01/07/2014 07:29 PM, David Holmes wrote:
On 7/01/2014 8:36 PM, Tristan Yan wrote:
Hi David
You're totally right. Sorry I ask you review it again.
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.02/
Looks good now.
Thanks,
David
Thank you very much.
Tristan
On 01/07/2014 05:18 PM, David Holmes wrote:
On 7/01/2014 6:16 PM, Tristan Yan wrote:
Thank you, David
I fixed copyright and change back sleep.
println was intended to be left in. This test was failed with timeout,
printf could help us to detect the value of total_turns_taken and
expected_turns_taken.
Please review it again
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.01/
Comma after 2014 still missing in copyright.
You need to read total_turns_taken.get() once and use that value in
both the println and the == test, so that you print the value you
actually tested.
David
Regards
Tristan
On 01/07/2014 03:10 PM, David Holmes wrote:
Hi Tristan,
On 7/01/2014 4:43 PM, Tristan Yan wrote:
Hi All
Please help to review the code change for JDK-7027502.
http://cr.openjdk.java.net/~tyan/JDK-7027502/
Description:
This test was failed in JPRT test but recently we test with same
binaries run, it doesn't fail any more. The intention for this code
change is bringing this test back to normal test and make this test
robust and more informative. Change includes
1. Remove this test from ProblemList.
2. Change static member total_turns_taken into a local variable.
Please let me know your comment on this.
2 * Copyright (c) 2004,2014 Oracle and/or its affiliates. All
rights reserved.
Correct copyright format should have a space before 2014 and a comma
after:
* Copyright (c) 2004, 2014, Oracle and/or its affiliates. All rights
reserved.
---
Was this println intended to be left in?
114 System.out.println(total_turns_taken = +
total_turns_taken +
115 ;expected_turns_taken = +
expected_turns_taken);
116 if ( total_turns_taken.get() ==
expected_turns_taken ) {
You only want to read total_turns_taken once otherwise you may see
misleading print outs.
---
119 /* Create some monitor contention by sleeping
with
lock */
120 if ( default_contention_sleep 0 ) {
121 System.out.println(Context sleeping, to
create contention);
122 try {
123 turn.wait((long)
default_contention_sleep);
124 } catch (InterruptedException ignore) { }
125 }
By changing the Thread.sleep to turn.wait you no longer introduce any
contention as the wait() will release the monitor.
David
Thank you.
Tristan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
hg: jdk8/tl/jdk: 8031187: DoubleStream.count is incorrect for a stream containing Integer.MAX_VALUE elements
Changeset: 630a92015993 Author:psandoz Date: 2014-01-09 10:52 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/630a92015993 8031187: DoubleStream.count is incorrect for a stream containing Integer.MAX_VALUE elements Reviewed-by: darcy ! src/share/classes/java/util/stream/DoublePipeline.java ! src/share/classes/java/util/stream/IntPipeline.java + test/java/util/stream/test/org/openjdk/tests/java/util/stream/CountLargeTest.java + test/java/util/stream/test/org/openjdk/tests/java/util/stream/CountTest.java
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.03/ I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. Regards Tristan On 01/09/2014 06:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 9/01/2014 8:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Now that you mention it ... I had mistakenly thought the switch to an AtomicInteger was to ensure the ++ was actually atomic, but as you note these updates all take place within a synchronized block that locks the same turn instance of TurnChecker. So now I'm confused by the change. Tristan what was the basis of making the change that you did ? If the issue was that concurrent hprof tests all used the same Context class in the same VM (how??) then as Paul says the TurnChecker instance would also be a problem. Otherwise the change makes no real difference to the semantics of the test as far as I can see. David Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On Jan 9, 2014, at 12:07 PM, Tristan Yan tristan@oracle.com wrote: Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. The suggestion (feel free to ignore it!) was to take the local variable: 161 int turns_taken = 0; and move it to a field on Context. Then in the synchronized block of the main loop sum 'em up. e.g.: ListContext cs = ; ... int current_total_turns_taken = cs.stream().mapToInt(c - c.turns_taken).sum(); Obviously that is less efficient but it does separate concerns. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 9/01/2014 9:07 PM, Tristan Yan wrote: Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.03/ Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. But you can't pass a reference to a simple int, for total_turns_taken, hence you turned it into the only mutable Integer type: AtomicInteger. I'm okay with this final form of the change. othervm would have been simpler :) These are ugly tests even with your changes. BTW: 107 Thread.yield(); 108 Thread.sleep(sleepTime); The yield() before the sleep is completely pointless. David - I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. Regards Tristan On 01/09/2014 06:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 9/01/2014 9:27 PM, David Holmes wrote: On 9/01/2014 9:07 PM, Tristan Yan wrote: Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.03/ Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. But you can't pass a reference to a simple int, for total_turns_taken, hence you turned it into the only mutable Integer type: AtomicInteger. I'm okay with this final form of the change. othervm would have been simpler :) These are ugly tests even with your changes. BTW: 107 Thread.yield(); 108 Thread.sleep(sleepTime); The yield() before the sleep is completely pointless. Ditto for: 126 Thread.yield(); 127 Thread.sleep((long)default_contention_sleep); David - David - I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. Regards Tristan On 01/09/2014 06:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Thanks David I removed those pointless yield, is it okay you can sponsor this for me? http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.04/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.04/ Regards Tristan On 01/09/2014 07:34 PM, David Holmes wrote: On 9/01/2014 9:27 PM, David Holmes wrote: On 9/01/2014 9:07 PM, Tristan Yan wrote: Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.03/ Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. But you can't pass a reference to a simple int, for total_turns_taken, hence you turned it into the only mutable Integer type: AtomicInteger. I'm okay with this final form of the change. othervm would have been simpler :) These are ugly tests even with your changes. BTW: 107 Thread.yield(); 108 Thread.sleep(sleepTime); The yield() before the sleep is completely pointless. Ditto for: 126 Thread.yield(); 127 Thread.sleep((long)default_contention_sleep); David - David - I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. Regards Tristan On 01/09/2014 06:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. -Alan.
Re: JDK 9 RFR of 8030814: Long.parseUnsignedLong should throw exception on too large input
On Jan 9, 2014, at 1:48 AM, Paul Sandoz wrote: This generally looks good, rather clever trick, i prefer it to using a cache. I agree with Joe some comments are required as it is not immediately obvious how this works. I have some simpler comments almost done being redacted. The additional tests seem adequate (overflow of the overflow), it's easy to go overboard e.g. you could test: BigInteger b = quotient.multiply(BigInteger.valueOf(radix + N)); for some range of N according to the radix under test. However, it might be useful to accurately test boundary conditions. Working on that also. Thanks, Brian
Re: JDK 9 RFR for 8031068: java/util/logging/ParentLoggersTest.java: checkLoggers: getLoggerNames() returned unexpected loggers
On 1/9/14 7:38 PM, Mandy Chung wrote: On 1/9/2014 10:18 AM, Daniel Fuchs wrote: OK - I removed the unnecessary calls. http://cr.openjdk.java.net/~dfuchs/webrev_8031068-jdk9/webrev.02/ Thumbs up! Thanks for the update. You have fixed a few test failures of this issue. Do you know if there is any other logging test that should be fixed? Yeah. That's what of the tests that had been identified by Seán in https://bugs.openjdk.java.net/browse/JDK-8029595. Hopefully there's only one left now - though I may have to review my analysis as I did it before discovering that holding the logger in a local variable was not enough to prevent garbage collection... -- daniel Mandy
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 9/01/2014 10:14 PM, Alan Bateman wrote: On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. That aside DemoRun forks off its own JVM to run a given test anyway! So I don't understand how the proposed fixes could actually be addressing the hangs that are occurring. Even if the statics were being shared I don't see how that leads to the failure mode in the bug report. David -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Hi David I wasn't able to reproduce this failure either in local or in our same binaries running(This is a continuous running with same JDK binaries). So intention for this code change is bringing this test back; add some debug info and try to avoid possible issues in this test. I agree this code change won't solve the failure happened. But this test was put into ProblemList two years ago better move for this is move out it from ProblemList and trace down the issue in our normal nightly. Thank you Tristan On 01/10/2014 06:35 AM, David Holmes wrote: On 9/01/2014 10:14 PM, Alan Bateman wrote: On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. That aside DemoRun forks off its own JVM to run a given test anyway! So I don't understand how the proposed fixes could actually be addressing the hangs that are occurring. Even if the statics were being shared I don't see how that leads to the failure mode in the bug report. David -Alan.
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
David/Peter you are right, the logs trace came from passed run, i am
trying to simulate the failure and get the logs for failed runs(2000+
runs done and still no failure), will get back to you once i have the
data from failed run. Sorry for the confusion.
---
Thanks
kalyan
On 01/08/2014 11:22 PM, David Holmes wrote:
Thanks Peter.
Kalyan: Can you confirm, as Peter asked, that the TraceExceptions
output came from a failed run?
AFAICS the Trace info is printed after each bytecode where there is a
pending exception - though I'm not 100% sure on the printing within
the VM runtime. Based on that I think we see the Trace output in run()
at the point where wait() returns, so it may well be caught after that
- in which case this was not a failing run.
I also can't reproduce the problem :(
David
On 8/01/2014 10:34 PM, Peter Levart wrote:
On 01/08/2014 07:30 AM, David Holmes wrote:
On 8/01/2014 4:19 PM, David Holmes wrote:
On 8/01/2014 7:33 AM, srikalyan chandrashekar wrote:
Hi David, TraceExceptions with fastdebug build produced some nice
trace
http://cr.openjdk.java.net/%7Esrikchan/OOME_exception_trace.log
. The
native method wait(long) is where the OOME if being thrown, the
deepest
call is in
src/share/vm/gc_interface/collectedHeap.inline.hpp, line 157
Yes but it is the caller that is of interest:
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown
[/HUDSON/workspace/8-2-build-linux-amd64/jdk8/1317/hotspot/src/share/vm/runtime/objectMonitor.cpp,
line 1649]
for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b4800ae0} 'wait'
'(J)V' in 'java/lang/Object'
at bci 0 for thread 0x7f78c40d2800
The ReferenceHandler thread gets the OOME trying to allocate the
InterruptedException.
However we already have a catch block around the wait() so how is this
OOME getting through? A bug in exception handling in the interpreter ??
Might be. And it may have something to do with the fact that the
Thread.run() method is the 1st call frame on the thread's stack (seems
like corner case). The last few meaningful TraceExceptions records are:
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown
[/HUDSON/workspace/8-2-build-linux-amd64/jdk8/1317/hotspot/src/share/vm/gc_interface/collectedHeap.inline.hpp,
line 157]
for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown
[/HUDSON/workspace/8-2-build-linux-amd64/jdk8/1317/hotspot/src/share/vm/runtime/objectMonitor.cpp,
line 1649]
for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b4800ae0} 'wait'
'(J)V' in 'java/lang/Object'
at bci 0 for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b4800ca8} 'wait'
'()V' in 'java/lang/Object'
at *bci 2* for thread 0x7f78c40d2800
Exception a 'java/lang/OutOfMemoryError' (0xd6a01840)
thrown in interpreter method {method} {0x7f78b48d2250} 'run'
'()V' in 'java/lang/ref/Reference$ReferenceHandler'
at *bci 36* for thread 0x7f78c40d2800
Here's the relevant bytecodes:
public class java.lang.Object
public final void wait() throws java.lang.InterruptedException;
descriptor: ()V
flags: ACC_PUBLIC, ACC_FINAL
Code:
stack=3, locals=1, args_size=1
0: aload_0
1: lconst_0
* 2: invokevirtual #73 // Method wait:(J)V*
5: return
LineNumberTable:
line 502: 0
line 503: 5
Exceptions:
throws java.lang.InterruptedException
class java.lang.ref.Reference$ReferenceHandler extends java.lang.Thread
public void run();
descriptor: ()V
flags: ACC_PUBLIC
Code:
stack=2, locals=5, args_size=1
0: invokestatic #62 // Method
java/lang/ref/Reference.access$100:()Ljava/lang/ref/Reference$Lock;
3: dup
4: astore_2
5: monitorenter
6: invokestatic #61 // Method
java/lang/ref/Reference.access$200:()Ljava/lang/ref/Reference;
9: ifnull33
12: invokestatic #61 // Method
java/lang/ref/Reference.access$200:()Ljava/lang/ref/Reference;
15: astore_1
16: aload_1
17: invokestatic #64 // Method
java/lang/ref/Reference.access$300:(Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference;
20: invokestatic #63 // Method
java/lang/ref/Reference.access$202:(Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference;
23: pop
24: aload_1
25: aconst_null
26: invokestatic #65 // Method
java/lang/ref/Reference.access$302:(Ljava/lang/ref/Reference;Ljava/lang/ref/Reference;)Ljava/lang/ref/Reference;
29: pop
RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 -Dan
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Looks ok to me. I presume getThreadStateValues is simply no longer needed. -Chris. On 10 Jan 2014, at 06:31, Dan Xu dan...@oracle.com wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 -Dan
