[9] Review request: JDK-8035640 JNU_CHECK_EXCEPTION should support c++ JNI syntax
Hello, Please review the fix for the issue: https://bugs.openjdk.java.net/browse/JDK-8035640 The fix is available at: http://cr.openjdk.java.net/~pchelko/9/8035640/webrev.00/ In AWT code we have quite a lot of C++ sources, but JNU_CHECK_EXCEPTION macros could not be used there, because the JNI syntax is different in C++. If approved I'll integrate this fix into the client forest, because we need this in client to fix parfait issues. Thank you, With best regards. Petr.
Re: RFR for JDK-8031374: java/util/concurrent/ConcurrentQueues/OfferRemoveLoops.java fails Intermittently
Tristan, Can you please file a new bug and bring in the changes that Martin pushed to the 166 CVS. http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/jtreg/util/concurrent/ConcurrentQueues/OfferRemoveLoops.java?view=co Create a changeset, and I can do the OpenJDK review. -Chris. On 24 Feb 2014, at 03:03, Tristan Yan tristan@oracle.com wrote: I am ok with this unfix for now. Martin. And thank you for bringing the improvement to jsr166 CVS. Regards Tristan 发件人: Martin Buchholz [mailto:marti...@google.com] 发送时间: Monday, February 24, 2014 10:59 AM 收件人: Tristan Yan 抄送: core-libs-dev 主题: Re: 答复: 答复: RFR for JDK-8031374: java/util/concurrent/ConcurrentQueues/OfferRemoveLoops.java fails Intermittently Hi Tristan, I don't think we should change the test without a sufficiently solid reason, which usually means reproducing the failure, even if e.g. only once in 1000 times. Here we don't know which queue implementation is causing the failure, and we don't know what chunk size was being used, which might cause the failure to be reproduced more reliably. A single test failure might be due to cosmic rays! Let's leave unfixed this while it cannot be reproduced. You can check in my general purpose improvements to the test from jsr166 CVS. On Sun, Feb 23, 2014 at 6:41 PM, Tristan Yan HYPERLINK mailto:tristan@oracle.comtristan@oracle.com wrote: Hi Martin This test failed once in our nightly test. So this is a real case failure. But unfortunately we couldn’t know more except the log we had. 10 extra seconds may need serve 1024 loop totally, also in windows Thread.yield() doesn’t give up CPU most of times, then we can may have the situation that remover is trying to remove object from a empty queue but it doesn’t find anything a couple of times(it doesn’t give up cpu time) then offer thread get cpu time. And offer thread insert a couple of elements and queue is full. Offer thread tries to give up CPU but Thread.yield() doesn’t really give up. Let’s assume it takes 1024 loop again. And offer thread got timeout. Then remover thread try to remove elements, and it takes another 1024 loop to get to timeout as well. So 10 seconds may need distribute to 2048 loop at most. Every loop has 5 ms foreach. That’s why I simulate this case with a Thread.sleep(20). I admit I can’t reproduce it without changing code, this is all theoretical analysis. But this is closest possible reason that I can deduce. Thank you. Tristan 发件人: Martin Buchholz [mailto:HYPERLINK mailto:marti...@google.commarti...@google.com] 发送时间: Monday, February 24, 2014 10:16 AM 收件人: Tristan Yan 抄送: core-libs-dev 主题: Re: 答复: RFR for JDK-8031374: java/util/concurrent/ConcurrentQueues/OfferRemoveLoops.java fails Intermittently I may very well be missing something, but the actual extra timeout for threads to finish is 10 whole seconds, which should be long enough to process a single chunk, even on Windows. If you can repro this consistently, try to find out which queue implementation is affected. We can also shrink max queue size and chunk size to reduce time to traverse the queue elements. On Sun, Feb 23, 2014 at 4:23 PM, Tristan Yan HYPERLINK mailto:tristan@oracle.comtristan@oracle.com wrote: Thank you for reviewing this. Martin The original bug report is here: https://bugs.openjdk.java.net/browse/JDK-8031374. I haven’t reproduced this bug but I can simulate to reproduce this failure by changing yield() in remover thread to Thread.sleep(20). You have commented “You've completely broken the intent of this code, which was to poll ***occasionally*** for quitting time”. I tried to not changed the logic for the test. This failure comes when only 1st rounds(1024 loop) wasn’t finished in given quitting time(before timeout). Which was 300ms. One solution is increasing default timeout as you suggested. But question is how big should we increase. When the test is really slow and could not finish 1st round(1024 loop) before timeout, I couldn’t figure out what’s the reason timeout value. Also this definitely will slow down the tests when it run in fast machine. Which is the case most of time. So I took other step that skip wait if test doesn't finish 1st round(1024 loop) before timeout. I won’t say I completely broke the intent of the code here because it’s rare case.(Only if the machine is slow and slow enough that the test doesn't finish 1st round before timeout). The reason that replace Thread.yield to Thread.sleep(0L) because David Holmes point out in the bug “sleep will certainly give up the CPU, whereas yield is not required to.” Also in other mail, David pointed I should use Thread.sleep(10L) instead of Thread.sleep(0L) preferably a bit longer as we don't know how the OS will behave if the sleep
Re: JDK 9 RFR of JDK-8035453: Fix serial lint warnings in com.sun.tools and elsewhere
Hi Joe,
On Feb 20, 2014, at 10:14 PM, Joe Darcy joe.da...@oracle.com wrote:
Hello,
Please review my proposed changes for:
JDK-8035453: Fix serial lint warnings in com.sun.tools and elsewhere
http://cr.openjdk.java.net/~darcy/8035453.0/webrev/
Looks good.
I agree with Paul B. that it seems a little odd to have to suppress
serialization on an abstract class, especially one that implements Serializable
(plus Connector.Argument also extends Serializable):
--- old/src/share/classes/com/sun/tools/jdi/ConnectorImpl.java 2014-02-20
13:03:08.0 -0800
+++ new/src/share/classes/com/sun/tools/jdi/ConnectorImpl.java 2014-02-20
13:03:08.0 -0800
@@ -144,6 +144,7 @@
return string;
}
+@SuppressWarnings(serial) // JDK implementation class
abstract class ArgumentImpl implements Connector.Argument, Cloneable,
Serializable {
private String name;
private String label;
Paul.
Re: [9] Review request: JDK-8035640 JNU_CHECK_EXCEPTION should support c++ JNI syntax
On 24/02/2014 09:02, Petr Pchelko wrote: Hello, Please review the fix for the issue: https://bugs.openjdk.java.net/browse/JDK-8035640 The fix is available at: http://cr.openjdk.java.net/~pchelko/9/8035640/webrev.00/ In AWT code we have quite a lot of C++ sources, but JNU_CHECK_EXCEPTION macros could not be used there, because the JNI syntax is different in C++. If approved I'll integrate this fix into the client forest, because we need this in client to fix parfait issues. Thank you, With best regards. Petr. This looks okay to me. One suggestion is to use #endif /* __cplusplus */ so that it's consistent with the other usages (also makes it a bit easier when there are nested ifdefs). As regards logistics then jdk9/dev might be the more suitable forest to push this to. I suggest this because it looks to me that jdk9/client is pulling down changes from jdk9/dev very regularly (which is good). On the other hand there doesn't appear to be regular integrations from jdk9/client to jdk9/dev yet. I see changes in jdk9/client from mid-December that has still not been pushed to jdk9/dev. It's just a suggestion to ensure that the changes get to both forests in timely manner. -Alan.
Re: [9] Review request: JDK-8035640 JNU_CHECK_EXCEPTION should support c++ JNI syntax
Hello, Alan. Thank you for the review. This looks okay to me. One suggestion is to use #endif /* __cplusplus */ so that it's consistent with the other usages (also makes it a bit easier when there are nested ifdefs). Updated the fix: http://cr.openjdk.java.net/~pchelko/9/8035640/webrev.01/ As regards logistics then jdk9/dev might be the more suitable forest to push this to. I suggest this because it looks to me that jdk9/client is pulling down changes from jdk9/dev very regularly (which is good). On the other hand there doesn't appear to be regular integrations from jdk9/client to jdk9/dev yet. I see changes in jdk9/client from mid-December that has still not been pushed to jdk9/dev. It's just a suggestion to ensure that the changes get to both forests in timely manner. No problem. I think we could easily wait until the next integration while dependent fixes are being reviewed. I'll push this into dev forest. With best regards. Petr. On 24.02.2014, at 16:10, Alan Bateman alan.bate...@oracle.com wrote: On 24/02/2014 09:02, Petr Pchelko wrote: Hello, Please review the fix for the issue: https://bugs.openjdk.java.net/browse/JDK-8035640 The fix is available at: http://cr.openjdk.java.net/~pchelko/9/8035640/webrev.00/ In AWT code we have quite a lot of C++ sources, but JNU_CHECK_EXCEPTION macros could not be used there, because the JNI syntax is different in C++. If approved I'll integrate this fix into the client forest, because we need this in client to fix parfait issues. Thank you, With best regards. Petr. This looks okay to me. One suggestion is to use #endif /* __cplusplus */ so that it's consistent with the other usages (also makes it a bit easier when there are nested ifdefs). As regards logistics then jdk9/dev might be the more suitable forest to push this to. I suggest this because it looks to me that jdk9/client is pulling down changes from jdk9/dev very regularly (which is good). On the other hand there doesn't appear to be regular integrations from jdk9/client to jdk9/dev yet. I see changes in jdk9/client from mid-December that has still not been pushed to jdk9/dev. It's just a suggestion to ensure that the changes get to both forests in timely manner. -Alan.
Re: [7u backport] RFR: 7122142: (ann) Race condition between isAnnotationPresent and getAnnotations
Hi Dmeetry, On 02/22/2014 01:22 PM, dmeetry degrave wrote: Hi all, I would like to ask for a review of combined back port for 7u-dev/7u80. The main goal is to have a fix for 7122142 in jdk7, it also integrates the changes from 8005232, 7185456, 8022721 https://bugs.openjdk.java.net/browse/JDK-7122142 https://bugs.openjdk.java.net/browse/JDK-8005232 https://bugs.openjdk.java.net/browse/JDK-7185456 https://bugs.openjdk.java.net/browse/JDK-8022721 Original jdk8 changes: 7122142: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e4ce6502eac0 8005232: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92 7185456: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae03282ba501 8022721: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2281a7f79738 back port: http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.0/ Patches can't be applied cleanly, hence it was a manual back port, though the final result is equivalent to applying the patches in chronological order (8005232, 7185456, 7122142, 8022721) and applying all the relevant rejected parts It's good to see those patches being back-ported to 7u. By browsing the webrev, I don't see any obvious difference between the original patches and the backport. Do you happen to remember in what part of code there were rejects so that you had to manually apply the changes? (with one exception, AnnotationTypeRuntimeAssumptionTest.java test was not included due to jdk8 API). Ah, It's the Class.getDeclaredAnnotation(Class) that's new in JDK8. Here's the changed test that only uses the JDK7 API so you can include this test too: http://cr.openjdk.java.net/~plevart/jdk7u/7122142/AnnotationTypeRuntimeAssumptionTest.java All tests in test/java/lang/annotation passed. thanks, dmeetry Regards, Peter
Re: RFR - 6857566 (bf) DirectByteBuffer garbage creation can outpace reclamation
On 24/02/2014 13:46, Peter Levart wrote: On 02/23/2014 09:09 AM, Alan Bateman wrote: http://cr.openjdk.java.net/~plevart/jdk9-dev/DirectBufferAlloc/webrev.03/ Thanks for the updates, this is good work and I think this is good to go. Do I need another official approval? Changes to the jdk repository only require one Reviewer (I think it's just the hotspot repository where it requires two). -Alan
答复: RFR for JDK-8031374: java/util/concurrent/ConcurrentQueues/OfferRemoveLoops.java fails Intermittently
Thank you for taking care of this. Chris Filed a bug for this https://bugs.openjdk.java.net/browse/JDK-8035661 Tristan -邮件原件- 发件人: Chris Hegarty 发送时间: Monday, February 24, 2014 5:05 PM 收件人: Tristan Yan 抄送: Martin Buchholz; core-libs-dev 主题: Re: RFR for JDK-8031374: java/util/concurrent/ConcurrentQueues/OfferRemoveLoops.java fails Intermittently Tristan, Can you please file a new bug and bring in the changes that Martin pushed to the 166 CVS. http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/jtreg/util/concurrent/ConcurrentQueues/OfferRemoveLoops.java?view=co Create a changeset, and I can do the OpenJDK review. -Chris. On 24 Feb 2014, at 03:03, Tristan Yan tristan@oracle.com wrote: I am ok with this unfix for now. Martin. And thank you for bringing the improvement to jsr166 CVS. Regards Tristan 发件人: Martin Buchholz [mailto:marti...@google.com] 发送时间: Monday, February 24, 2014 10:59 AM 收件人: Tristan Yan 抄送: core-libs-dev 主题: Re: 答复: 答复: RFR for JDK-8031374: java/util/concurrent/ConcurrentQueues/OfferRemoveLoops.java fails Intermittently Hi Tristan, I don't think we should change the test without a sufficiently solid reason, which usually means reproducing the failure, even if e.g. only once in 1000 times. Here we don't know which queue implementation is causing the failure, and we don't know what chunk size was being used, which might cause the failure to be reproduced more reliably. A single test failure might be due to cosmic rays! Let's leave unfixed this while it cannot be reproduced. You can check in my general purpose improvements to the test from jsr166 CVS. On Sun, Feb 23, 2014 at 6:41 PM, Tristan Yan HYPERLINK mailto:tristan@oracle.comtristan@oracle.com wrote: Hi Martin This test failed once in our nightly test. So this is a real case failure. But unfortunately we couldn’t know more except the log we had. 10 extra seconds may need serve 1024 loop totally, also in windows Thread.yield() doesn’t give up CPU most of times, then we can may have the situation that remover is trying to remove object from a empty queue but it doesn’t find anything a couple of times(it doesn’t give up cpu time) then offer thread get cpu time. And offer thread insert a couple of elements and queue is full. Offer thread tries to give up CPU but Thread.yield() doesn’t really give up. Let’s assume it takes 1024 loop again. And offer thread got timeout. Then remover thread try to remove elements, and it takes another 1024 loop to get to timeout as well. So 10 seconds may need distribute to 2048 loop at most. Every loop has 5 ms foreach. That’s why I simulate this case with a Thread.sleep(20). I admit I can’t reproduce it without changing code, this is all theoretical analysis. But this is closest possible reason that I can deduce. Thank you. Tristan 发件人: Martin Buchholz [mailto:HYPERLINK mailto:marti...@google.commarti...@google.com] 发送时间: Monday, February 24, 2014 10:16 AM 收件人: Tristan Yan 抄送: core-libs-dev 主题: Re: 答复: RFR for JDK-8031374: java/util/concurrent/ConcurrentQueues/OfferRemoveLoops.java fails Intermittently I may very well be missing something, but the actual extra timeout for threads to finish is 10 whole seconds, which should be long enough to process a single chunk, even on Windows. If you can repro this consistently, try to find out which queue implementation is affected. We can also shrink max queue size and chunk size to reduce time to traverse the queue elements. On Sun, Feb 23, 2014 at 4:23 PM, Tristan Yan HYPERLINK mailto:tristan@oracle.comtristan@oracle.com wrote: Thank you for reviewing this. Martin The original bug report is here: https://bugs.openjdk.java.net/browse/JDK-8031374. I haven’t reproduced this bug but I can simulate to reproduce this failure by changing yield() in remover thread to Thread.sleep(20). You have commented “You've completely broken the intent of this code, which was to poll ***occasionally*** for quitting time”. I tried to not changed the logic for the test. This failure comes when only 1st rounds(1024 loop) wasn’t finished in given quitting time(before timeout). Which was 300ms. One solution is increasing default timeout as you suggested. But question is how big should we increase. When the test is really slow and could not finish 1st round(1024 loop) before timeout, I couldn’t figure out what’s the reason timeout value. Also this definitely will slow down the tests when it run in fast machine. Which is the case most of time. So I took other step that skip wait if test doesn't finish 1st round(1024 loop) before timeout. I won’t say I completely broke the intent of the code here because it’s rare case.(Only if the machine is slow and slow enough that the test doesn't finish 1st round before timeout).
Re: RFR [6853696] (ref) ReferenceQueue.remove(timeout) may return null even if timeout has not expired
Hi Ivan,
The code is correct as written but there might be some creep in the end
time due to the sampling of System.nanoTime.
I would be inclined to calculate the final time of the timeout once
and then compare simply with the current nanotime.
long end = (timeout == 0) ? Long.MAX_VALUE : (System.nanoTime() +
timeout * 100);
Then the test in the loop can be:
if (System.nanoTime() end) {
return null;
}
Roger (Not a Reviewer)
On 2/24/2014 12:59 AM, Ivan Gerasimov wrote:
Hello!
ReferenceQueue.remove(timeout) may return too early, i.e. before the
specified timeout has elapsed.
Would you please review the fix?
The change also includes a regression test, which can be used to
demonstrate the issue.
BUGURL: https://bugs.openjdk.java.net/browse/JDK-6853696
WEBREV: http://cr.openjdk.java.net/~igerasim/6853696/0/webrev/
Sincerely yours,
Ivan
Re: RFR [6853696] (ref) ReferenceQueue.remove(timeout) may return null even if timeout has not expired
Thank you Roger for looking at the fix!
On 24.02.2014 18:37, roger riggs wrote:
Hi Ivan,
The code is correct as written but there might be some creep in the
end time due to the sampling of System.nanoTime.
I would be inclined to calculate the final time of the timeout once
and then compare simply with the current nanotime.
long end = (timeout == 0) ? Long.MAX_VALUE : (System.nanoTime() +
timeout * 100);
I don't think it is always correct.
According to the documentation, The value returned represents
nanoseconds since some fixed but *arbitrary* origin time.
http://download.java.net/jdk8/docs/api/java/lang/System.html#nanoTime--
Thus, Long.MAX_VALUE may just happen to represent some valid time in the
near future, and we'll achieve wrong result.
Sincerely yours,
Ivan
Then the test in the loop can be:
if (System.nanoTime() end) {
return null;
}
Roger (Not a Reviewer)
On 2/24/2014 12:59 AM, Ivan Gerasimov wrote:
Hello!
ReferenceQueue.remove(timeout) may return too early, i.e. before the
specified timeout has elapsed.
Would you please review the fix?
The change also includes a regression test, which can be used to
demonstrate the issue.
BUGURL: https://bugs.openjdk.java.net/browse/JDK-6853696
WEBREV: http://cr.openjdk.java.net/~igerasim/6853696/0/webrev/
Sincerely yours,
Ivan
Re: AWT Dev [9] Review request: JDK-8035640 JNU_CHECK_EXCEPTION should support c++ JNI syntax
Hi Petr, The fix looks fine to me. -- best regards, Anthony On 2/24/2014 4:21 PM, Petr Pchelko wrote: Hello, Alan. Thank you for the review. This looks okay to me. One suggestion is to use #endif /* __cplusplus */ so that it's consistent with the other usages (also makes it a bit easier when there are nested ifdefs). Updated the fix: http://cr.openjdk.java.net/~pchelko/9/8035640/webrev.01/ As regards logistics then jdk9/dev might be the more suitable forest to push this to. I suggest this because it looks to me that jdk9/client is pulling down changes from jdk9/dev very regularly (which is good). On the other hand there doesn't appear to be regular integrations from jdk9/client to jdk9/dev yet. I see changes in jdk9/client from mid-December that has still not been pushed to jdk9/dev. It's just a suggestion to ensure that the changes get to both forests in timely manner. No problem. I think we could easily wait until the next integration while dependent fixes are being reviewed. I'll push this into dev forest. With best regards. Petr. On 24.02.2014, at 16:10, Alan Bateman alan.bate...@oracle.com wrote: On 24/02/2014 09:02, Petr Pchelko wrote: Hello, Please review the fix for the issue: https://bugs.openjdk.java.net/browse/JDK-8035640 The fix is available at: http://cr.openjdk.java.net/~pchelko/9/8035640/webrev.00/ In AWT code we have quite a lot of C++ sources, but JNU_CHECK_EXCEPTION macros could not be used there, because the JNI syntax is different in C++. If approved I'll integrate this fix into the client forest, because we need this in client to fix parfait issues. Thank you, With best regards. Petr. This looks okay to me. One suggestion is to use #endif /* __cplusplus */ so that it's consistent with the other usages (also makes it a bit easier when there are nested ifdefs). As regards logistics then jdk9/dev might be the more suitable forest to push this to. I suggest this because it looks to me that jdk9/client is pulling down changes from jdk9/dev very regularly (which is good). On the other hand there doesn't appear to be regular integrations from jdk9/client to jdk9/dev yet. I see changes in jdk9/client from mid-December that has still not been pushed to jdk9/dev. It's just a suggestion to ensure that the changes get to both forests in timely manner. -Alan.
Re: RFR: JDK-8030780 - test/com/sun/corba/cachedSocket/7056731.sh leaves HelloServer behind
Looks ok to me Mark. -Chris. On 21/02/14 13:21, Mark Sheppard wrote: Hi please oblige and review the follow change http://cr.openjdk.java.net/~msheppar/8030780/webrev/ to address the issue raised in https://bugs.openjdk.java.net/browse/JDK-8030780 summary: if the the java debugger failed to connect to the client, then the sequence of commands which include kill -9 of the server process will not be executed. the server process is added to the post test kill In a successful execution this will result in a no such process message. regards Mark
RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Hello! We've got one quite old report about been unable to run the test under Android. https://bugs.openjdk.java.net/browse/JDK-6943190 That was due to hard-coded path to the cat utility as /bin/cat. When investigating, I found two other tests under the same directory that use /usr/bin/cat and /usr/bin/echo. These two test seem to (almost) never run because of the unusual path. Here's the first version of the webrev, with the fix to only three tests mentioned above: http://cr.openjdk.java.net/~igerasim/6943190/0/webrev/ java/lang/Runtime/exec/ directory has also got several other tests, which run shell commands. Some of them have absolute path and some of them don't. So I have a general question: Why would we ever need the absolute path for the commands? Why wouldn't we rely on the PATH env variable? Sincerely yours, Ivan
Re: [9] RFR (M): 8027827: Improve performance of catchException combinator
On Feb 20, 2014, at 9:57 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com wrote: Updated webrev: http://cr.openjdk.java.net/~vlivanov/8027827/final/webrev.01/ I finally figured out how to make caching work. This webrev contains these changes. I changed LF representation a bit and added 2 auxiliary method handles - argument boxing and wrapping into Object[] and result unboxing. These operations depend on actual type and can't be shared among arbitrary combinators with the same basic type. They are used only during LF interpretation and are completely ignored in compiled LFs. This is a good step forward, thanks! Some comments: I prefer the bounds check expression pos+3 lambdaForm.names.length. (One integer constant, limit to right.) The predicate isGuardWithCatch must test all three subforms. Or else there must be assertions to ensure that names[pos+2] is of the expected form. The problem is that LF's can sometimes be edited (e.g., by binding operations) and there is no insurance that your pattern of three expressions will be preserved in all cases. I see you are trying to do unboxing elimination here; this is not a safe or effective way to do it, IMO. Put in a FIXME comment and file a bug to deal better with unboxing ops in LFs. I have some WIP code toward this end which we can talk about. You've probably seen of that business, about internally LF marking expressions as intrinsics to guide bytecode generation. Why is the logic about cachedLambdaForm commented out? It looks correct, but is there a bug? Consider replacing GUARD_WITH_CATCH with Lazy.NF_guardWithCatch, and using the NF instead of MH for the intrinsic. — John
JEP 191: Foreign Function Interface
Posted: http://openjdk.java.net/jeps/191 - Mark
Re: RFR [6853696] (ref) ReferenceQueue.remove(timeout) may return null even if timeout has not expired
On Feb 24 2014, at 06:37 , roger riggs roger.ri...@oracle.com wrote:
Hi Ivan,
The code is correct as written but there might be some creep in the end time
due to the sampling of System.nanoTime.
I would be inclined to calculate the final time of the timeout once
and then compare simply with the current nanotime.
long end = (timeout == 0) ? Long.MAX_VALUE : (System.nanoTime() + timeout *
100);
I hate seeing numerical constants
TimeUnit.MILLISECONDS.toNanos(timeout)
Then the test in the loop can be:
if (System.nanoTime() end) {
return null;
}
This compare should be re-written in the overflow compensating style Martin
mentions.
Mike
Roger (Not a Reviewer)
On 2/24/2014 12:59 AM, Ivan Gerasimov wrote:
Hello!
ReferenceQueue.remove(timeout) may return too early, i.e. before the
specified timeout has elapsed.
Would you please review the fix?
The change also includes a regression test, which can be used to demonstrate
the issue.
BUGURL: https://bugs.openjdk.java.net/browse/JDK-6853696
WEBREV: http://cr.openjdk.java.net/~igerasim/6853696/0/webrev/
Sincerely yours,
Ivan
Re: [7u backport] RFR: 7122142: (ann) Race condition between isAnnotationPresent and getAnnotations
Thanks for looking at this, Peter! On 02/24/2014 04:42 PM, Peter Levart wrote: Hi Dmeetry, On 02/22/2014 01:22 PM, dmeetry degrave wrote: Hi all, I would like to ask for a review of combined back port for 7u-dev/7u80. The main goal is to have a fix for 7122142 in jdk7, it also integrates the changes from 8005232, 7185456, 8022721 https://bugs.openjdk.java.net/browse/JDK-7122142 https://bugs.openjdk.java.net/browse/JDK-8005232 https://bugs.openjdk.java.net/browse/JDK-7185456 https://bugs.openjdk.java.net/browse/JDK-8022721 Original jdk8 changes: 7122142: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e4ce6502eac0 8005232: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92 7185456: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae03282ba501 8022721: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2281a7f79738 back port: http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.0/ Patches can't be applied cleanly, hence it was a manual back port, though the final result is equivalent to applying the patches in chronological order (8005232, 7185456, 7122142, 8022721) and applying all the relevant rejected parts It's good to see those patches being back-ported to 7u. By browsing the webrev, I don't see any obvious difference between the original patches and the backport. there shouldn't be any! Do you happen to remember in what part of code there were rejects so that you had to manually apply the changes? there were conflicts due to small difference between 7 and 8 (copyrights, white spaces, @SuppressWarnings, Class?,...). I copied all rejected parts and original patches here: http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/rej/ (with one exception, AnnotationTypeRuntimeAssumptionTest.java test was not included due to jdk8 API). Ah, It's the Class.getDeclaredAnnotation(Class) that's new in JDK8. Here's the changed test that only uses the JDK7 API so you can include this test too: http://cr.openjdk.java.net/~plevart/jdk7u/7122142/AnnotationTypeRuntimeAssumptionTest.java Thanks! http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/ (just with the new test added). thanks, dmeetry All tests in test/java/lang/annotation passed. thanks, dmeetry Regards, Peter
Re: RFR [6853696] (ref) ReferenceQueue.remove(timeout) may return null even if timeout has not expired
On 2/23/14 9:59 PM, Ivan Gerasimov wrote: Hello! ReferenceQueue.remove(timeout) may return too early, i.e. before the specified timeout has elapsed. Would you please review the fix? The change also includes a regression test, which can be used to demonstrate the issue. BUGURL: https://bugs.openjdk.java.net/browse/JDK-6853696 WEBREV: http://cr.openjdk.java.net/~igerasim/6853696/0/webrev/ I'll review the patch once you address the numerical overflow issue Martin points out. One minor comment - I suggest to use start and end instead of before and after. One comment on the test, line 61: I think you want to check thread.suspect expects to be true. It may be simply keeping the returned value of ReferenceQueue.remove call and check that in line 61 instead of having thread.suspect variable. line 76: why do you want to catch InterruptedException? If interrupted, should the test fail? Mandy
RFR 8027640 : String.indexOf(String,int) for the empty string case not specified
Please review my changes for: https://bugs.openjdk.java.net/browse/JDK-8027640 The webrev is here: http://cr.openjdk.java.net/~bchristi/8027640/webrev.00/ This is a small cleanup to reconcile the String[last]indexOf() spec with longstanding behavior. There are no changes to executable code. In String, the Math.min(fromIndex, this.length()) clause is brought over from String[Buffer|Builder] for indexOf(String,int) and lastIndexOf(String,int). This resolves the issue for which the bug was filed. To further sync up the [last]indexOf() docs, I ported the 6940381 wording improvements from String to String[Buffer|Builder]. I also made a few doclint updates to the code in question. Thanks, -Brent
Re: JDK 9 RFR of JDK-8035279: Clean up internal deprecations in BigInteger
On Feb 20, 2014, at 6:39 PM, David Holmes wrote: Not clear what you mean by this. This is more or less my reaction to this entire thread, so to speak. ;-) Anyway, thanks for all the comments. Note that I am ignoring the powerCache field comments for the moment. To distill the discussion down to just the proposed changes I posted, my reading is that there is more or less consensus on two points: 1) the instance fields in question *should* be volatile for this proposed change set 2) non-zero initial values should be avoided in case of instance leaking to non-constructing threads Is this accurate? On second thought it occurred to me that instead of any complicated or contentious changes, as it were, the ugliness I was trying to remove from the code could just as well be addressed by simply changing the names of the affected instance variables to indicate what their respective values really represent, e.g., bitCount becomes bitCountPlusOne and we remove the @Deprecated and @deprecated. Yeah this is still ugly ... Thanks, Brian
Re: JDK 9 RFR of JDK-8035453: Fix serial lint warnings in com.sun.tools and elsewhere
Hi Paul, On 02/24/2014 03:23 AM, Paul Sandoz wrote: Hi Joe, On Feb 20, 2014, at 10:14 PM, Joe Darcyjoe.da...@oracle.com wrote: bHello, Please review my proposed changes for: JDK-8035453: Fix serial lint warnings in com.sun.tools and elsewhere http://cr.openjdk.java.net/~darcy/8035453.0/webrev/ Looks good. I agree with Paul B. that it seems a little odd to have to suppress serialization on an abstract class, especially one that implements Serializable (plus Connector.Argument also extends Serializable): Despite cleaning up many serial warnings, I've tried hard to avoid learning too much about the serialization wire protocol ;-) From what I was able to discern by reading the serialization specification [1], If a class does *not* declare a serialVersionUID, the serialVersionUID of its superclass is *not* included in the serialver hash computation of the child class. However, my understanding is that changes to the fields stored in superclass and changes to the semantics of its readObject / writeObjects methods could affect the serialization of the child class. Therefore, if that understanding is correct, it seems reasonable for javac to warn about serializable abstract classes even though one cannot have bare instances of the class. Thanks for the review, -Joe [1] http://docs.oracle.com/javase/7/docs/platform/serialization/spec/class.html#4100
Re: RFR [6853696] (ref) ReferenceQueue.remove(timeout) may return null even if timeout has not expired
On 25/02/2014 1:34 AM, Ivan Gerasimov wrote:
Thank you Roger for looking at the fix!
On 24.02.2014 18:37, roger riggs wrote:
Hi Ivan,
The code is correct as written but there might be some creep in the
end time due to the sampling of System.nanoTime.
I would be inclined to calculate the final time of the timeout once
and then compare simply with the current nanotime.
long end = (timeout == 0) ? Long.MAX_VALUE : (System.nanoTime() +
timeout * 100);
I don't think it is always correct.
According to the documentation, The value returned represents
nanoseconds since some fixed but *arbitrary* origin time.
http://download.java.net/jdk8/docs/api/java/lang/System.html#nanoTime--
Thus, Long.MAX_VALUE may just happen to represent some valid time in the
near future, and we'll achieve wrong result.
Correct. Simplest thing is to calculate end ignoring whether timeout is
zero, and only check now end if timeout != 0.
David
Sincerely yours,
Ivan
Then the test in the loop can be:
if (System.nanoTime() end) {
return null;
}
Roger (Not a Reviewer)
On 2/24/2014 12:59 AM, Ivan Gerasimov wrote:
Hello!
ReferenceQueue.remove(timeout) may return too early, i.e. before the
specified timeout has elapsed.
Would you please review the fix?
The change also includes a regression test, which can be used to
demonstrate the issue.
BUGURL: https://bugs.openjdk.java.net/browse/JDK-6853696
WEBREV: http://cr.openjdk.java.net/~igerasim/6853696/0/webrev/
Sincerely yours,
Ivan
Re: RFR [6853696] (ref) ReferenceQueue.remove(timeout) may return null even if timeout has not expired
On 25.02.2014 6:52, David Holmes wrote:
On 25/02/2014 1:34 AM, Ivan Gerasimov wrote:
Thank you Roger for looking at the fix!
On 24.02.2014 18:37, roger riggs wrote:
Hi Ivan,
The code is correct as written but there might be some creep in the
end time due to the sampling of System.nanoTime.
I would be inclined to calculate the final time of the timeout once
and then compare simply with the current nanotime.
long end = (timeout == 0) ? Long.MAX_VALUE : (System.nanoTime() +
timeout * 100);
I don't think it is always correct.
According to the documentation, The value returned represents
nanoseconds since some fixed but *arbitrary* origin time.
http://download.java.net/jdk8/docs/api/java/lang/System.html#nanoTime--
Thus, Long.MAX_VALUE may just happen to represent some valid time in the
near future, and we'll achieve wrong result.
Correct. Simplest thing is to calculate end ignoring whether timeout
is zero, and only check now end if timeout != 0.
David, as Martin said before, 'now end' may hold true even if now is
after end (now is negative).
In my version of the code, I operate on the difference (after - before),
thus avoiding the possible overflow.
Sincerely yours,
Ivan
David
Sincerely yours,
Ivan
Then the test in the loop can be:
if (System.nanoTime() end) {
return null;
}
Roger (Not a Reviewer)
On 2/24/2014 12:59 AM, Ivan Gerasimov wrote:
Hello!
ReferenceQueue.remove(timeout) may return too early, i.e. before the
specified timeout has elapsed.
Would you please review the fix?
The change also includes a regression test, which can be used to
demonstrate the issue.
BUGURL: https://bugs.openjdk.java.net/browse/JDK-6853696
WEBREV: http://cr.openjdk.java.net/~igerasim/6853696/0/webrev/
Sincerely yours,
Ivan
Re: JDK 9 RFR of JDK-8035452: Fix serial lint warnings in core libs
Hi Paul,
On 02/20/2014 12:49 PM, Paul Benedict wrote:
Joe, I find it interesting that you suppressed the serial warning on
an abstract class. I'd like to know more about that. Is this a
universal rule? Are serial uids not important for abstract classes?
I wouldn't generalize that way from this example.
The serial hash of NavigableSubMap has differed in different JDK
releases, but its subclasses do define serialVersionUID fields. I assume
the set of non-transient fields in NavigableSubMap has stayed unchanged,
but I haven't verified that.
If I hadn't found the change in the hash value, I would have added the
serialVersionUID to NavigableSubMap too.
HTH,
-Joe
On Thu, Feb 20, 2014 at 2:33 PM, Joe Darcy joe.da...@oracle.com
mailto:joe.da...@oracle.com wrote:
Hello,
Please review the patch below which will address the handful of
remaining serial lint warning in the core libraries.
Thanks,
-Joe
diff -r e5a09bc70308 src/share/classes/java/util/EnumSet.java
--- a/src/share/classes/java/util/EnumSet.javaThu Feb 20
13:03:36 2014 +
+++ b/src/share/classes/java/util/EnumSet.javaThu Feb 20
12:32:52 2014 -0800
@@ -80,6 +80,8 @@
public abstract class EnumSetE extends EnumE extends
AbstractSetE
implements Cloneable, java.io.Serializable
{
+private static final long serialVersionUID =
1009687484059888093L;
+
/**
* The class of all the elements of this set.
*/
diff -r e5a09bc70308 src/share/classes/java/util/TreeMap.java
--- a/src/share/classes/java/util/TreeMap.javaThu Feb 20
13:03:36 2014 +
+++ b/src/share/classes/java/util/TreeMap.javaThu Feb 20
12:32:52 2014 -0800
@@ -1333,6 +1333,7 @@
/**
* @serial include
*/
+@SuppressWarnings(serial) // Abstract class
abstract static class NavigableSubMapK,V extends
AbstractMapK,V
implements NavigableMapK,V, java.io.Serializable {
/**
diff -r e5a09bc70308
src/share/classes/sun/reflect/annotation/ExceptionProxy.java
--- a/src/share/classes/sun/reflect/annotation/ExceptionProxy.java
Thu Feb 20 13:03:36 2014 +
+++ b/src/share/classes/sun/reflect/annotation/ExceptionProxy.java
Thu Feb 20 12:33:21 2014 -0800
@@ -37,5 +37,6 @@
* @since 1.5
*/
public abstract class ExceptionProxy implements
java.io.Serializable {
+private static final long serialVersionUID =
7241930048386631401L;
protected abstract RuntimeException generateException();
}
--
Cheers,
Paul
Re: RFR [6853696] (ref) ReferenceQueue.remove(timeout) may return null even if timeout has not expired
On 25/02/2014 1:49 PM, Ivan Gerasimov wrote:
On 25.02.2014 6:52, David Holmes wrote:
On 25/02/2014 1:34 AM, Ivan Gerasimov wrote:
Thank you Roger for looking at the fix!
On 24.02.2014 18:37, roger riggs wrote:
Hi Ivan,
The code is correct as written but there might be some creep in the
end time due to the sampling of System.nanoTime.
I would be inclined to calculate the final time of the timeout once
and then compare simply with the current nanotime.
long end = (timeout == 0) ? Long.MAX_VALUE : (System.nanoTime() +
timeout * 100);
I don't think it is always correct.
According to the documentation, The value returned represents
nanoseconds since some fixed but *arbitrary* origin time.
http://download.java.net/jdk8/docs/api/java/lang/System.html#nanoTime--
Thus, Long.MAX_VALUE may just happen to represent some valid time in the
near future, and we'll achieve wrong result.
Correct. Simplest thing is to calculate end ignoring whether timeout
is zero, and only check now end if timeout != 0.
David, as Martin said before, 'now end' may hold true even if now is
after end (now is negative).
In my version of the code, I operate on the difference (after - before),
thus avoiding the possible overflow.
Sorry, I put now end in quotes to mean the effect of nowend, the
actual calculation should still be a subtraction as Martin indicated.
However I was overlooking the fact you need to reduce the timeout for
subsequent waits.
David
Sincerely yours,
Ivan
David
Sincerely yours,
Ivan
Then the test in the loop can be:
if (System.nanoTime() end) {
return null;
}
Roger (Not a Reviewer)
On 2/24/2014 12:59 AM, Ivan Gerasimov wrote:
Hello!
ReferenceQueue.remove(timeout) may return too early, i.e. before the
specified timeout has elapsed.
Would you please review the fix?
The change also includes a regression test, which can be used to
demonstrate the issue.
BUGURL: https://bugs.openjdk.java.net/browse/JDK-6853696
WEBREV: http://cr.openjdk.java.net/~igerasim/6853696/0/webrev/
Sincerely yours,
Ivan
Re: JDK 9 RFR of JDK-8035452: Fix serial lint warnings in core libs
On 2/24/14 8:22 PM, Joe Darcy wrote: On 02/20/2014 12:49 PM, Paul Benedict wrote: Joe, I find it interesting that you suppressed the serial warning on an abstract class. I'd like to know more about that. Is this a universal rule? Are serial uids not important for abstract classes? I wouldn't generalize that way from this example. The serial hash of NavigableSubMap has differed in different JDK releases, but its subclasses do define serialVersionUID fields. I assume the set of non-transient fields in NavigableSubMap has stayed unchanged, but I haven't verified that. If I hadn't found the change in the hash value, I would have added the serialVersionUID to NavigableSubMap too. And in his reply to Paul, Joe said: From what I was able to discern by reading the serialization specification [1], If a class does *not* declare a serialVersionUID, the serialVersionUID of its superclass is *not* included in the serialver hash computation of the child class. However, my understanding is that changes to the fields stored in superclass and changes to the semantics of its readObject / writeObjects methods could affect the serialization of the child class. I think we need to take a closer look at these issues. I believe that abstract, serializable superclasses *do* need to have a serialVersionUID defined. The reason is that when a subclass is serialized, its superclass descriptor (an ObjectStreamClass) is also serialized. Upon deserialization, the descriptor's svuid is matched against the svuid of the class loaded at runtime, and if there is a mismatch, InvalidClassException ensues. While the svuid of an abstract superclass isn't included in the subclass' svuid hash, the svuid of the superclass does affect serial compatibility of subclasses as described above. Thus, an apparently innocuous change to the superclass might prevent serial compatibility of its subclasses, no matter how carefully the subclasses are programmed. If the NavigableSubMap class has changed svuid values over several releases, well, unfortunately we may have a compatibility problem already in the field. We'd need to choose which release to be compatible with. Since 8 isn't quite out yet, we might be able to change an early 8-update and 9 to be compatibile with the latest 7-update. Note that the svuid of a class does not relate solely to the fields that are serialized. It's an attempt at a version hash of the *implementation* of a class, not a version of the serial protocol. Even changes to a class that don't affect the serialized output stream can affect the svuid. For example, renaming a package-private method will affect the svuid. See section 4.6 of the serialization spec. While we're at it (sorry...) in the diffs for your other serial warnings patch JDK-8035453, there are several lines where the serial warning is suppressed like so: +@SuppressWarnings(serial) // JDK implementation class As you know, serialization can expose the private fields of a class, making them public in a sense. Serialization can also expose what are internal, implementation classes, if these classes are part of a serializable object graph that is exposed to applications. I don't know about the specific situation with the DOM classes, but even if a serializable class is internal, we might need to be concerned about serialization compatibility. Finally, EnumSet doesn't need a serial version UID. It's serialized using a proxy class, so EnumSet never appears in a serialized byte stream. (Note, its readObject throws an exception unconditionally.) So it's probably safe to suppress its serialization warning. s'marks
