Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/10/2014 12:59 AM, srikalyan chandrashekar wrote:
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.
I doubt the logs will be any different. A simple test that throws an
exception inside Thread.run() without catching it shows that
TraceExceptions doesn't report the fact that Thread.run() terminates
abruptly (as David pointed out, pending exception is reported after
every bytecode executed and there's no bytecode that invoked Thread.run()).
While you're at it, testing, could you also test the modified
ReferenceHandler (the one that calls private runImpl() from it's run()
method) so that we get a proof of incorrect behaviour.
Since we suspect there's something wrong with exception handling in
interpreter, I devised a hypothetical reproducer that tries to simulate
ReferenceHandler in many aspects, but doesn't require to be a
ReferenceHandler:
http://cr.openjdk.java.net/~plevart/misc/OOME/OOMECatchingTest.java
This is designed to run indefinitely and only terminate if/when thread
dies. Could you run this program in the environment that causes the
OOMEInReferenceHandler test to fail and see if it terminates?
Regards, Peter
---
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
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 09/01/2014 23:20, Tristan Yan wrote: 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. If we can't duplicate it now, and the output from previously reported failures (in 2011) is insufficient to diagnose it properly, then it seems reasonable to add better output so as to improve our chances of understanding if it fails again. So better output + removing from the exclude list seems fine here. (cc'ing serviceability-dev as that is actually the mailing list for the HPROF and JVM TI areas). -Alan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 10 jan 2014, at 09:34, Alan Bateman alan.bate...@oracle.com wrote: On 09/01/2014 23:20, Tristan Yan wrote: 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. If we can't duplicate it now, and the output from previously reported failures (in 2011) is insufficient to diagnose it properly, then it seems reasonable to add better output so as to improve our chances of understanding if it fails again. So better output + removing from the exclude list seems fine here. (cc'ing serviceability-dev as that is actually the mailing list for the HPROF and JVM TI areas). Sounds good to me, /Staffan -Alan
RFR 8031428 CountTest causes lambda Ser/Derialization tests to fail
Hi,
A small tweak is required to a recent Stream-based test i added to stop some
internal lambda-based ser/derialization (SAND) tests barfing since the test is
hostile to ser/derialization, and infact i should have probably written the
test like below in the first place.
Kumar has verified it fixes the SAND test failures.
Paul.
https://bugs.openjdk.java.net/browse/JDK-8031428
diff -r e332a6819993
test/java/util/stream/test/org/openjdk/tests/java/util/stream/CountTest.java
---
a/test/java/util/stream/test/org/openjdk/tests/java/util/stream/CountTest.java
Fri Jan 10 08:22:00 2014 +0100
+++
b/test/java/util/stream/test/org/openjdk/tests/java/util/stream/CountTest.java
Fri Jan 10 10:58:06 2014 +0100
@@ -29,7 +29,6 @@
package org.openjdk.tests.java.util.stream;
-import java.util.concurrent.atomic.AtomicLong;
import java.util.stream.DoubleStream;
import java.util.stream.DoubleStreamTestDataProvider;
import java.util.stream.IntStream;
@@ -47,45 +46,41 @@
@Test(dataProvider = StreamTestDataInteger, dataProviderClass =
StreamTestDataProvider.class)
public void testOps(String name, TestData.OfRefInteger data) {
-AtomicLong expectedCount = new AtomicLong();
-data.stream().forEach(e - expectedCount.incrementAndGet());
+long expectedCount = data.size();
withData(data).
terminal(Stream::count).
-expectedResult(expectedCount.get()).
+expectedResult(expectedCount).
exercise();
}
@Test(dataProvider = IntStreamTestData, dataProviderClass =
IntStreamTestDataProvider.class)
public void testOps(String name, TestData.OfInt data) {
-AtomicLong expectedCount = new AtomicLong();
-data.stream().forEach(e - expectedCount.incrementAndGet());
+long expectedCount = data.size();
withData(data).
terminal(IntStream::count).
-expectedResult(expectedCount.get()).
+expectedResult(expectedCount).
exercise();
}
@Test(dataProvider = LongStreamTestData, dataProviderClass =
LongStreamTestDataProvider.class)
public void testOps(String name, TestData.OfLong data) {
-AtomicLong expectedCount = new AtomicLong();
-data.stream().forEach(e - expectedCount.incrementAndGet());
+long expectedCount = data.size();
withData(data).
terminal(LongStream::count).
-expectedResult(expectedCount.get()).
+expectedResult(expectedCount).
exercise();
}
@Test(dataProvider = DoubleStreamTestData, dataProviderClass =
DoubleStreamTestDataProvider.class)
public void testOps(String name, TestData.OfDouble data) {
-AtomicLong expectedCount = new AtomicLong();
-data.stream().forEach(e - expectedCount.incrementAndGet());
+long expectedCount = data.size();
withData(data).
terminal(DoubleStream::count).
-expectedResult(expectedCount.get()).
+expectedResult(expectedCount).
exercise();
}
}
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
On 10/01/2014 06:31, Dan Xu 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 This looks good, the only one that isn't clear (to me) is the GetStringLength usage in MessageUtil.c where I don't think it is possible to ever get 0. This may be a case where you need to use ExceptionOccured instead. -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 10/01/2014 6:40 PM, Staffan Larsen wrote: On 10 jan 2014, at 09:34, Alan Bateman alan.bate...@oracle.com wrote: On 09/01/2014 23:20, Tristan Yan wrote: 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. If we can't duplicate it now, and the output from previously reported failures (in 2011) is insufficient to diagnose it properly, then it seems reasonable to add better output so as to improve our chances of understanding if it fails again. So better output + removing from the exclude list seems fine here. (cc'ing serviceability-dev as that is actually the mailing list for the HPROF and JVM TI areas). Sounds good to me, I'm not sure what is actually being proposed. I don't really see anything that would help diagnoze the original issue. But bring back the test - maybe this was a bug elsewhere that has now been fixed. David /Staffan -Alan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 10/01/2014 10:37, David Holmes wrote: I'm not sure what is actually being proposed. I don't really see anything that would help diagnoze the original issue. But bring back the test - maybe this was a bug elsewhere that has now been fixed. I think the proposal is as we said, more diagnostic output + remove from exclude list. If Tristan's agrees then I'm sure he'll update the patch. On your second point then you may be right, there were a number of issues during JDK 8 and it's very possible that the ghost being chasing now is gone. -Alan.
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
On 01/10/2014 09:31 AM, Peter Levart wrote: Since we suspect there's something wrong with exception handling in interpreter, I devised a hypothetical reproducer that tries to simulate ReferenceHandler in many aspects, but doesn't require to be a ReferenceHandler: http://cr.openjdk.java.net/~plevart/misc/OOME/OOMECatchingTest.java This is designed to run indefinitely and only terminate if/when thread dies. Could you run this program in the environment that causes the OOMEInReferenceHandler test to fail and see if it terminates? I forgot to mention that in order for this long-running program to exhibit interpreter behaviour, it should be run with -Xint option. So I suggest: -Xmx24M -XX:-UseTLAB -Xint Regards, Peter
RFR 8030848: Collections.sort(List l, Comparator) should defer to List.sort(Comparator )
Hi, When we added the List.sort method the default implementation deferred to Collections.sort. This is the wrong way around, since any existing use of Collections.sort say with ArrayList will not avail of the optimal implementation provided by ArrayList.sort. To resolve this the implementation of Collections.sort can be moved to List.sort and Collections.sort can defer to List.sort. Code changes are here: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8030848-Collections-sort/webrev/ I made some tweaks to Arrays.sort to preserve cases when the Comparator is null. Existing tests provide good coverage and there are no regressions when i run j.u. tests locally. I am not happy with the current documentation though, i think that also requires some refactoring, moving stuff from Collections.sort to List.sort and explicitly stating what the current implementation of Collections.sort does. I believe this requires no spec changes even though it may appear so. Thoughts? Also, i am concerned that this change could cause stack overflows for list implementations that override sort and still defer to Collections.sort. Implying we should fix this for 8 or quickly in an 8u. Paul.
Re: RFR 8031428 CountTest causes lambda Ser/Derialization tests to fail
Looks fine to me. I don't think AtomicLong was ever needed here.
-Chris.
On 10/01/14 10:01, Paul Sandoz wrote:
Hi,
A small tweak is required to a recent Stream-based test i added to stop some
internal lambda-based ser/derialization (SAND) tests barfing since the test is
hostile to ser/derialization, and infact i should have probably written the
test like below in the first place.
Kumar has verified it fixes the SAND test failures.
Paul.
https://bugs.openjdk.java.net/browse/JDK-8031428
diff -r e332a6819993
test/java/util/stream/test/org/openjdk/tests/java/util/stream/CountTest.java
---
a/test/java/util/stream/test/org/openjdk/tests/java/util/stream/CountTest.java
Fri Jan 10 08:22:00 2014 +0100
+++
b/test/java/util/stream/test/org/openjdk/tests/java/util/stream/CountTest.java
Fri Jan 10 10:58:06 2014 +0100
@@ -29,7 +29,6 @@
package org.openjdk.tests.java.util.stream;
-import java.util.concurrent.atomic.AtomicLong;
import java.util.stream.DoubleStream;
import java.util.stream.DoubleStreamTestDataProvider;
import java.util.stream.IntStream;
@@ -47,45 +46,41 @@
@Test(dataProvider = StreamTestDataInteger, dataProviderClass =
StreamTestDataProvider.class)
public void testOps(String name, TestData.OfRefInteger data) {
-AtomicLong expectedCount = new AtomicLong();
-data.stream().forEach(e - expectedCount.incrementAndGet());
+long expectedCount = data.size();
withData(data).
terminal(Stream::count).
-expectedResult(expectedCount.get()).
+expectedResult(expectedCount).
exercise();
}
@Test(dataProvider = IntStreamTestData, dataProviderClass =
IntStreamTestDataProvider.class)
public void testOps(String name, TestData.OfInt data) {
-AtomicLong expectedCount = new AtomicLong();
-data.stream().forEach(e - expectedCount.incrementAndGet());
+long expectedCount = data.size();
withData(data).
terminal(IntStream::count).
-expectedResult(expectedCount.get()).
+expectedResult(expectedCount).
exercise();
}
@Test(dataProvider = LongStreamTestData, dataProviderClass =
LongStreamTestDataProvider.class)
public void testOps(String name, TestData.OfLong data) {
-AtomicLong expectedCount = new AtomicLong();
-data.stream().forEach(e - expectedCount.incrementAndGet());
+long expectedCount = data.size();
withData(data).
terminal(LongStream::count).
-expectedResult(expectedCount.get()).
+expectedResult(expectedCount).
exercise();
}
@Test(dataProvider = DoubleStreamTestData, dataProviderClass =
DoubleStreamTestDataProvider.class)
public void testOps(String name, TestData.OfDouble data) {
-AtomicLong expectedCount = new AtomicLong();
-data.stream().forEach(e - expectedCount.incrementAndGet());
+long expectedCount = data.size();
withData(data).
terminal(DoubleStream::count).
-expectedResult(expectedCount.get()).
+expectedResult(expectedCount).
exercise();
}
}
Re: RFR 8031428 CountTest causes lambda Ser/Derialization tests to fail
On Jan 10, 2014, at 12:48 PM, Chris Hegarty chris.hega...@oracle.com wrote: Looks fine to me. Thanks. I don't think AtomicLong was ever needed here. It was crudely used as a holder of the count, since captured refs are (effectively) final, and not for it's concurrent properties as i wanted to avoid using any higher-level operations such as sum(). Paul.
Re: (reflect) Accessing members of inner annotations types
Hi, Many thanks for the investigation and suggested workarounds. Bean Validation recommends to declare an inner @List annotation to specify several constraints of the same type on a given element, but its ok to deviate from that pattern in the rare cases of package-private constraint types. I can file a bug/rfe for this. I think that would be great; Taking the referenced types into account when determining the package for generated proxy classes sounds reasonable to me. Thanks again, --Gunnar 2014/1/8 Joel Borggren-Franck joel.fra...@oracle.com On 2014-01-08, Peter Levart wrote: On 01/08/2014 01:00 PM, Joel Borggren-Franck wrote: Perhaps an alternative would be to check if the interface a proxy is proxying is nested. In that case use the outermost interface's access level for the package calculation. This would probably lead to a few more proxies being generated into the real package compared to your proposal. I don't know if that is ok or not. The nested public interface need not be referencing the outer class/interface. In this case, proxy class can be generated in com.sun.proxy. Yes, this is the reason for my remark about more proxies being generated into the real package. I'm not sure this is a problem, however it is moot considering you second point. More importantly, even if the outer interface/class was public, the nested interface could be referencing some other package-private type. For example: Ugh. Acknowledged. (This feels like a really bad practice but that doesn't matter here.) cheers /Joel
RFR 8029452: Fork/Join task ForEachOps.ForEachOrderedTask clarifications and minor improvements
Hi, Some tweaks to the Stream forEachOrdered operation: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8029452-ForEachOrdered/webrev/ The first tweak is to size the CHM used in ForEachOrderedTask, this avoids concurrent resizes and the costs associated with those. The second tweak is to consolidate the reporting of elements to within the ForEachOrderedTask.tryComplete method. I have also removed the inconsistently applied synchronized block. Either we apply it consistently to reporting or not at all. It was originally there because we were not sure that the happens-before relationship [1] between elements would be guaranteed. However, ForEachOrderedTask sets up such a relationship via completion counts to ensure leaf nodes complete in encounter order (if any) where only one leaf can be completing (which was left most leaf that was not completed), hence stamping a fence in the ground at these point seems redundant (at least i cannot see its value but could be missing something subtle). Paul. [1] * pThis operation processes the elements one at a time, in encounter * order if one exists. Performing the action for one element * a href=../concurrent/package-summary.html#MemoryVisibilityihappens-before/i/a * performing the action for subsequent elements, but for any given element, * the action may be performed in whatever thread the library chooses. * * @param action a a href=package-summary.html#NonInterference * non-interfering/a action to perform on the elements * @see #forEach(Consumer) */ void forEachOrdered(Consumer? super T action);
Re: RFR 8030848: Collections.sort(List l, Comparator) should defer to List.sort(Comparator )
On 10/01/2014 11:21, Paul Sandoz wrote: Hi, When we added the List.sort method the default implementation deferred to Collections.sort. This is the wrong way around, since any existing use of Collections.sort say with ArrayList will not avail of the optimal implementation provided by ArrayList.sort. To resolve this the implementation of Collections.sort can be moved to List.sort and Collections.sort can defer to List.sort. Code changes are here: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8030848-Collections-sort/webrev/ I made some tweaks to Arrays.sort to preserve cases when the Comparator is null. Existing tests provide good coverage and there are no regressions when i run j.u. tests locally. I am not happy with the current documentation though, i think that also requires some refactoring, moving stuff from Collections.sort to List.sort and explicitly stating what the current implementation of Collections.sort does. I believe this requires no spec changes even though it may appear so. Thoughts? Also, i am concerned that this change could cause stack overflows for list implementations that override sort and still defer to Collections.sort. Implying we should fix this for 8 or quickly in an 8u. Paul. The implementation changes look good. I agree that the javadoc needs changing as it's otherwise misleading as to what the implementation actually does. I would think that this should go with the implementation change rather than as a separate change. So is the stack overflow concern with List implementations that were originally developed to target JDK 7 or older? In any case, this is the type of change more suitable to a major release. -Alan.
Request for approval for bug #8031488
Hello, I would like to request for approval for this fix. This is simple revert of the changes which caused the issue. I’ve returned back synchronization and removed volatile. So now serialVersionUID is the same as before. Bug: https://bugs.openjdk.java.net/browse/JDK-8031488 Webrev: http://cr.openjdk.java.net/~mkos/8027908/webrev.00 Thank you. — Best regardsю Iaroslav
Re: Request for approval for bug #8031488
Hi, Alan, You are absolutely right. Unfortunately the things a little bit more complicated. The reason why I’m fixing this now is, that some time ago I fix this synchronization issue (synchronized setter without synchronized getter). After that I got this bug. We had internal discussions if I can leave my changes and the short answer is “no” :| Because it’s JAXB API and I can’t change signatures within the same version. So I have to revert my changes and leave it as it was before. We will fix this in the next MR for JAXB API. -- Regards. Iaroslav On 10 Jan 2014, at 15:52, Alan Bateman alan.bate...@oracle.com wrote: On 10/01/2014 14:26, Iaroslav Savytskyi wrote: Hello, I would like to request for approval for this fix. This is simple revert of the changes which caused the issue. I’ve returned back synchronization and removed volatile. So now serialVersionUID is the same as before. Bug: https://bugs.openjdk.java.net/browse/JDK-8031488 Webrev: http://cr.openjdk.java.net/~mkos/8027908/webrev.00 If these are changed to use synchronization then maybe you want to change getLinkedException too. In any case, isn't the right thing to just add the serialVersionUID? That is, I assume the issue that the missing serialVersionUID meant the default SUID changed when you changed a field modifier. -Alan.
RFR: (8030875) Macros for checking and returning on exceptions
Please review: To enable native code checking consistently for thrown exceptions, the macros in net_util.h and java/util/jar/pack/coding.cpp are made consolidated and promoted to jni_util.h webrev: http://cr.openjdk.java.net/~rriggs/webrev-check-exception-8030875/ [1] https://bugs.openjdk.java.net/browse/JDK-8030875
Re: Request for approval for bug #8031488
On 10/01/2014 15:08, Iaroslav Savytskyi wrote:
Hi, Alan,
You are absolutely right. Unfortunately the things a little bit more
complicated. The reason why I’m fixing this now is, that some time ago I fix
this synchronization issue (synchronized setter without synchronized getter).
After that I got this bug. We had internal discussions if I can leave my
changes and the short answer is “no” :| Because it’s JAXB API and I can’t
change signatures within the same version. So I have to revert my changes and
leave it as it was before. We will fix this in the next MR for JAXB API.
It looks to me that JAXBException has always defined its SUID so I
assume this means there isn't really any need to revert that, right?
For TypeConstraintException then adding the SUID to the value that it
has always been doesn't change anything except that it now appears in
the serialization form that javadoc reports. So any implementation of
this exception type would need to use that value. That might be why you
need to do a MR. If it is required then your change is okay although I
think it would be better if getLinkedException was synchronized. You can
use synchronized (this) { ... } around the read to avoid changing the
modifiers (and hence SUID).
-Alan.
Re: RFR: (8030875) Macros for checking and returning on exceptions
Thank you Roger, much appreciated. I think Dan has a change in flight that could be simplified a bit by using these. -Chris. On 10/01/14 15:37, roger riggs wrote: Please review: To enable native code checking consistently for thrown exceptions, the macros in net_util.h and java/util/jar/pack/coding.cpp are made consolidated and promoted to jni_util.h webrev: http://cr.openjdk.java.net/~rriggs/webrev-check-exception-8030875/ [1] https://bugs.openjdk.java.net/browse/JDK-8030875
Re: Request for approval for bug #8031488
On 10 Jan 2014, at 16:36, Alan Bateman alan.bate...@oracle.com wrote:
On 10/01/2014 15:08, Iaroslav Savytskyi wrote:
Hi, Alan,
You are absolutely right. Unfortunately the things a little bit more
complicated. The reason why I’m fixing this now is, that some time ago I fix
this synchronization issue (synchronized setter without synchronized
getter). After that I got this bug. We had internal discussions if I can
leave my changes and the short answer is “no” :| Because it’s JAXB API and I
can’t change signatures within the same version. So I have to revert my
changes and leave it as it was before. We will fix this in the next MR for
JAXB API.
It looks to me that JAXBException has always defined its SUID so I assume
this means there isn't really any need to revert that, right?
Yes, JAXBException has SUID. The reason I’ve reverted my changes is that I’m
going to fix both files at once in MR.
For TypeConstraintException then adding the SUID to the value that it has
always been doesn't change anything except that it now appears in the
serialization form that javadoc reports. So any implementation of this
exception type would need to use that value. That might be why you need to do
a MR. If it is required then your change is okay although I think it would be
better if getLinkedException was synchronized. You can use synchronized
(this) { ... } around the read to avoid changing the modifiers (and hence
SUID).
-Alan.
There are 3 possibilities:
1) Because it was my own initiative to fix this potential synchronization bug
and nobody didn’t report it, we can approve my fix and leave this 2 classes
without synchronized getters. And fix it in MR.
2) Fix it as you propose. But later we will definitely need to change it again
to volatile for performance reasons.
3) Leave classes with volatile as they are now. And only add SUID to
TypeConstraintException class.
—
Iaroslav
Re: RFR: (8030875) Macros for checking and returning on exceptions
On 10/01/2014 15:37, roger riggs wrote: Please review: To enable native code checking consistently for thrown exceptions, the macros in net_util.h and java/util/jar/pack/coding.cpp are made consolidated and promoted to jni_util.h webrev: http://cr.openjdk.java.net/~rriggs/webrev-check-exception-8030875/ This looks okay to me. It would be good if Kumar takes a look at this too because it results in a mix of macros in the pack code. -Alan
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu 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
JDK8 RFR JDK-8029237 Update copyright year to match last edit in jdk8 jaxws repository for 2013
Hi, this is about fixing copyright years for jdk8 (!); the incorrect years found for both 2012 and 2013, so not to confuse script jdk8/make/scripts/update_copyright_year.sh it is necessary to use option -d date for commit. I tested that and it seems to work perfect. I used following: [2012 modifications] hg commit -u mkos -l ../2012-msg.txt -d 2012-12-30 [2013 modifications] hg commit -u mkos -l ../2013-msg.txt -d 2013-12-30 When commits performed with past date, update_copyright_year.sh script finds those in proper years. Since I am not familiar with using webrev for pushing changes, I exported both revisions into separate patches: 2012: http://cr.openjdk.java.net/~mkos/8029237/copyrights-2012.patch 2013: http://cr.openjdk.java.net/~mkos/8029237/copyrights-2013.patch all changes webrev: http://cr.openjdk.java.net/~mkos/8029237/webrev-all.00/ (upload still in progress, I hope it doesn't take more than 30 mins) bug: https://bugs.openjdk.java.net/browse/JDK-8029237 Regards Miran
Re: RFR: (8030875) Macros for checking and returning on exceptions
On 10/01/14 15:37, roger riggs wrote: Please review: To enable native code checking consistently for thrown exceptions, the macros in net_util.h and java/util/jar/pack/coding.cpp are made consolidated and promoted to jni_util.h webrev: http://cr.openjdk.java.net/~rriggs/webrev-check-exception-8030875/ [1] https://bugs.openjdk.java.net/browse/JDK-8030875 Looks fine to me Michael.
Re: RFR: (8030875) Macros for checking and returning on exceptions
On 1/10/2014 7:37 AM, roger riggs wrote: Please review: To enable native code checking consistently for thrown exceptions, the macros in net_util.h and java/util/jar/pack/coding.cpp are made consolidated and promoted to jni_util.h webrev: http://cr.openjdk.java.net/~rriggs/webrev-check-exception-8030875/ Looks good. Thanks for doing this Roger. Minor nit: the convention in jni.h seems to name functions that take JNIEnv* parameter with the JNU_ prefix and so might be better to rename CHECK_EXCEPTION to JNU_CheckedException. Mandy
JDK-9 RFR for 8031525: Logger created in test/tools/jar/UpdateManifest.java might get gc'ed too early.
Hi, Please find below a trivial fix for 8031525: Logger created in test/tools/jar/UpdateManifest.java might get gc'ed too early. https://bugs.openjdk.java.net/browse/JDK-8031525 Hopefully that's the last of its kind - (see https://bugs.openjdk.java.net/browse/JDK-8029595). webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8031525-jdk9/webrev.00/ best regards, -- daniel
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Thanks, Chris. Right, I don't find any usage of getThreadStateValues, so it is simpler to just remove it. -Dan On 01/09/2014 11:49 PM, Chris Hegarty wrote: 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
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
On 01/10/2014 02:32 AM, Alan Bateman wrote: On 10/01/2014 06:31, Dan Xu 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 This looks good, the only one that isn't clear (to me) is the GetStringLength usage in MessageUtil.c where I don't think it is possible to ever get 0. This may be a case where you need to use ExceptionOccured instead. -Alan. According to jni.cpp, GetStringLength() will always return positive value or 0. For simplicity, I will change = 0 to == 0. Thanks, Alan. -Dan
Re: JDK-9 RFR for 8031525: Logger created in test/tools/jar/UpdateManifest.java might get gc'ed too early.
Looks good to me Daniel. -Chris. On 10 Jan 2014, at 17:47, Daniel Fuchs daniel.fu...@oracle.com wrote: Hi, Please find below a trivial fix for 8031525: Logger created in test/tools/jar/UpdateManifest.java might get gc'ed too early. https://bugs.openjdk.java.net/browse/JDK-8031525 Hopefully that's the last of its kind - (see https://bugs.openjdk.java.net/browse/JDK-8029595). webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8031525-jdk9/webrev.00/ best regards, -- daniel
Re: RFR 8030848: Collections.sort(List l, Comparator) should defer to List.sort(Comparator )
The implementation looks good. I would move construction of the listIterator to before the toArray() for detection of concurrent modification (growing of the list). I believe we should fix this for 8 if possible. Mike On Jan 10 2014, at 03:21 , Paul Sandoz paul.san...@oracle.com wrote: Hi, When we added the List.sort method the default implementation deferred to Collections.sort. This is the wrong way around, since any existing use of Collections.sort say with ArrayList will not avail of the optimal implementation provided by ArrayList.sort. To resolve this the implementation of Collections.sort can be moved to List.sort and Collections.sort can defer to List.sort. Code changes are here: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8030848-Collections-sort/webrev/ I made some tweaks to Arrays.sort to preserve cases when the Comparator is null. Existing tests provide good coverage and there are no regressions when i run j.u. tests locally. I am not happy with the current documentation though, i think that also requires some refactoring, moving stuff from Collections.sort to List.sort and explicitly stating what the current implementation of Collections.sort does. I believe this requires no spec changes even though it may appear so. Thoughts? Also, i am concerned that this change could cause stack overflows for list implementations that override sort and still defer to Collections.sort. Implying we should fix this for 8 or quickly in an 8u. Paul.
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Hi Dan, One other comment, instead of changing the return type of the setStaticIntField just return and the caller should check for exceptions and return. See jni.h: CHECK_EXCEPTION(env) Roger On 1/10/2014 11:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu 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
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Hi Roger, My macro is a little different from yours, which compares with -1 instead of NULL. I also see CHECK_EXCEPTION macro. Thanks for adding them, which are useful when I cannot decide the pending exception state by just using return values. As for the style, actually I prefer the (!pointer) to (pointer == NULL) because it is more concise and also make me avoid the typo like (pointer = NULL), which I cannot find from the compilation. Thanks! -Dan On 01/10/2014 08:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu 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
Re: RFR: (8030875) Macros for checking and returning on exceptions
Hi Mandy, Good point; I'll create another issue to do that update. (I should have waited a bit longer for comments.) Roger On 1/10/2014 12:41 PM, Mandy Chung wrote: On 1/10/2014 7:37 AM, roger riggs wrote: Please review: To enable native code checking consistently for thrown exceptions, the macros in net_util.h and java/util/jar/pack/coding.cpp are made consolidated and promoted to jni_util.h webrev: http://cr.openjdk.java.net/~rriggs/webrev-check-exception-8030875/ Looks good. Thanks for doing this Roger. Minor nit: the convention in jni.h seems to name functions that take JNIEnv* parameter with the JNU_ prefix and so might be better to rename CHECK_EXCEPTION to JNU_CheckedException. Mandy
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
On 10 Jan 2014, at 18:05, Dan Xu dan...@oracle.com wrote: Hi Roger, My macro is a little different from yours, which compares with -1 instead of NULL. I also see CHECK_EXCEPTION macro. Thanks for adding them, which are useful when I cannot decide the pending exception state by just using return values. As for the style, actually I prefer the (!pointer) to (pointer == NULL) because it is more concise and also make me avoid the typo like (pointer = NULL), which I cannot find from the compilation. Thanks! Not that it matters, but my preference is to == NULL. -Chris. -Dan On 01/10/2014 08:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu 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
Re: RFR 8029452: Fork/Join task ForEachOps.ForEachOrderedTask clarifications and minor improvements
On Jan 10 2014, at 05:42 , Paul Sandoz paul.san...@oracle.com wrote: Hi, Some tweaks to the Stream forEachOrdered operation: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8029452-ForEachOrdered/webrev/ The first tweak is to size the CHM used in ForEachOrderedTask, this avoids concurrent resizes and the costs associated with those. +1 The second tweak is to consolidate the reporting of elements to within the ForEachOrderedTask.tryComplete method. I have also removed the inconsistently applied synchronized block. Either we apply it consistently to reporting or not at all. It was originally there because we were not sure that the happens-before relationship [1] between elements would be guaranteed. However, ForEachOrderedTask sets up such a relationship via completion counts to ensure leaf nodes complete in encounter order (if any) where only one leaf can be completing (which was left most leaf that was not completed), hence stamping a fence in the ground at these point seems redundant (at least i cannot see its value but could be missing something subtle). Coud not the lock object be removed? Mike Paul. [1] * pThis operation processes the elements one at a time, in encounter * order if one exists. Performing the action for one element * a href=../concurrent/package-summary.html#MemoryVisibilityihappens-before/i/a * performing the action for subsequent elements, but for any given element, * the action may be performed in whatever thread the library chooses. * * @param action a a href=package-summary.html#NonInterference * non-interfering/a action to perform on the elements * @see #forEach(Consumer) */ void forEachOrdered(Consumer? super T action);
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Yes, you are right. I just found this macro. It looks very handy to use. Thanks! -Dan On 01/10/2014 09:59 AM, roger riggs wrote: Hi Dan, One other comment, instead of changing the return type of the setStaticIntField just return and the caller should check for exceptions and return. See jni.h: CHECK_EXCEPTION(env) Roger On 1/10/2014 11:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu 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
Re: RFR 8029452: Fork/Join task ForEachOps.ForEachOrderedTask clarifications and minor improvements
On Jan 10, 2014, at 7:11 PM, Mike Duigou mike.dui...@oracle.com wrote: The second tweak is to consolidate the reporting of elements to within the ForEachOrderedTask.tryComplete method. I have also removed the inconsistently applied synchronized block. Either we apply it consistently to reporting or not at all. It was originally there because we were not sure that the happens-before relationship [1] between elements would be guaranteed. However, ForEachOrderedTask sets up such a relationship via completion counts to ensure leaf nodes complete in encounter order (if any) where only one leaf can be completing (which was left most leaf that was not completed), hence stamping a fence in the ground at these point seems redundant (at least i cannot see its value but could be missing something subtle). Coud not the lock object be removed? Doh, yes, thanks, updated, Paul.
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
On Jan 10 2014, at 10:09 , Chris Hegarty chris.hega...@oracle.com wrote: On 10 Jan 2014, at 18:05, Dan Xu dan...@oracle.com wrote: Hi Roger, My macro is a little different from yours, which compares with -1 instead of NULL. I also see CHECK_EXCEPTION macro. Thanks for adding them, which are useful when I cannot decide the pending exception state by just using return values. As for the style, actually I prefer the (!pointer) to (pointer == NULL) because it is more concise and also make me avoid the typo like (pointer = NULL), which I cannot find from the compilation. Thanks! There's always yoda conditions https://en.wikipedia.org/wiki/Yoda_conditions, (NULL == pointer), but that's not likely to make anyone (besides me) happy. Mike Not that it matters, but my preference is to == NULL. -Chris. -Dan On 01/10/2014 08:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu 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
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Thank you for all the feedback. I have updated my changes to use CHECK_EXCEPTION_RETURN and CHECK_EXCEPTION macro recently added into jni_util.h. I also removed else block in function setStaticIntField() in Version.c since (*env)-GetStaticFieldID will throw a same exception if the field cannot be found. Here is the new webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.01/. Thanks! -Dan On 01/10/2014 10:21 AM, Mike Duigou wrote: On Jan 10 2014, at 10:09 , Chris Hegarty chris.hega...@oracle.com wrote: On 10 Jan 2014, at 18:05, Dan Xu dan...@oracle.com wrote: Hi Roger, My macro is a little different from yours, which compares with -1 instead of NULL. I also see CHECK_EXCEPTION macro. Thanks for adding them, which are useful when I cannot decide the pending exception state by just using return values. As for the style, actually I prefer the (!pointer) to (pointer == NULL) because it is more concise and also make me avoid the typo like (pointer = NULL), which I cannot find from the compilation. Thanks! There's always yoda conditions https://en.wikipedia.org/wiki/Yoda_conditions, (NULL == pointer), but that's not likely to make anyone (besides me) happy. Mike Not that it matters, but my preference is to == NULL. -Chris. -Dan On 01/10/2014 08:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu 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
Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently
Hi Peter the version you provided ran indefinitely(i put a 10 minute timeout) and the program got interrupted(no error), even if there were to be an error you cannot print the string of thread to console(these have been attempted earlier). - The test's running on interpreter mode, what i am watching for is one error with trace. Without fastdebug build and -XX:+TraceExceptions i am able to reproduce failure atleast 5 failures out of 1000 runs but with fastdebug+Trace no luck yet(already past few 1000 runs). --- Thanks kalyan On 01/10/2014 02:57 AM, Peter Levart wrote: On 01/10/2014 09:31 AM, Peter Levart wrote: Since we suspect there's something wrong with exception handling in interpreter, I devised a hypothetical reproducer that tries to simulate ReferenceHandler in many aspects, but doesn't require to be a ReferenceHandler: http://cr.openjdk.java.net/~plevart/misc/OOME/OOMECatchingTest.java This is designed to run indefinitely and only terminate if/when thread dies. Could you run this program in the environment that causes the OOMEInReferenceHandler test to fail and see if it terminates? I forgot to mention that in order for this long-running program to exhibit interpreter behaviour, it should be run with -Xint option. So I suggest: -Xmx24M -XX:-UseTLAB -Xint Regards, Peter
8031494: [launcher] java launcher should check for JNI Pending exceptions.
Hi, Please review fixes for launcher correctness wrt. JNI calls. http://cr.openjdk.java.net/~ksrini/8031494/webrev.0/ Thanks Kumar
