Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 30/07/2015 8:41 PM, Peter Levart wrote:
On 07/30/2015 12:24 PM, David Holmes wrote:
On 30/07/2015 8:20 PM, Peter Levart wrote:
On 07/30/2015 11:12 AM, Daniel Fuchs wrote:
Hi Peter,
I'm glad you're looking at this too! We can't have too many eyes :-)
On 30/07/15 10:38, Peter Levart wrote:
Suppose we have a Reference 'r' and it's associated ReferenceQueue
'q'.
Currently it can happen that the following evaluates to true, which is
surprising:
q.poll() == null r.isEnqueued()
But on the other hand this can only happen if two different
threads are polling the queue - in which case only one of them
will get the reference. In such a situation, the symmetric condition
would not be surprising (as the other thread would
get q.poll() != null):
r.isEnqueued() q.poll() == null
The original bug fixed by Kim is more surprising, because there's only
one Thread doing the polling, and it does get:
r.isEnqueud() q.poll() == null
although nobody else is polling the queue.
This should no longer occur in this situation with Kim's fix.
cheers,
-- daniel
Yes, these are two different issues. The one Kim has fixed is the race
of above expressions with Queue.enqueue() (when the queue is changing
from the associated instance to ENQUEUED). The one I'm pointing at and
Kim has already identified as potential issue is the race of the
following expression:
r.isEnqueued() q.poll() == null r.isEnqueued() == true
with Queue.[realy]poll() (when the queue is changing from ENQUEUED
to NULL).
Which only happens if at least two threads are polling the queue, but is
equally surprising and prone to cause bugs, don't you think?
Sorry I'm missing your point. The expression:
r.isEnqueued() q.poll() == null
is exactly the race the current fix is addressing. Adding a second
check of r.isEnqueued() which still returns true does not add anything
that I can see. ??
David
The expressions are similar, but the race is different. The one
addressed by Kim is the race of:
r.isEnqueued() q.poll() == null == true
with q.enqueue(r)
There, the reversal of assignments to q.head and r.queue in
ReferenceQueue.enqueue() fixes the issue.
The one I'm pointing at is the race of:
r.isEnqueued() q.poll() == null r.isEnqueued() == true
with q.poll()
Here, the fix would be to also revert the assignments to q.head and
r.queue in ReferenceQueue.reallyPoll() this time.
The 1st race is the race with enqueue-ing and the 2nd race is the race
with de-queueing. Initially, my surprising expression was:
q.poll() == null r.isEnqueued() == true
...but this is not representative, as it is also an expected outcome
when racing with enqueue-ing. So I added a pre-condition to express the
fact that it happens when racing with de-queueing only:
r.isEnqueued() q.poll() == null r.isEnqueued() == true
What is surprising in the above expression evaluating to true is the
fact that 'r' appears to be enqueued before and after the q.poll()
returns null. I can easily imagine code that would fail because it never
imagined above expression to evaluate to true. For example:
So r has been enqueued and one poll() removes it, so the second poll()
returns NULL, but r still claims to be enqueued. Sorry I'm not seeing
how that is possible.
David
if (r.isEnqueued()) {
Reference s = q.poll();
if (s == null) {
// somebody else already dequeued 'r'
assert !r.isEnqueued();
}
}
Regards, Peter
Regards, Peter
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 30/07/2015 9:57 PM, Peter Levart wrote:
On 07/30/2015 01:44 PM, David Holmes wrote:
r.isEnqueued() q.poll() == null r.isEnqueued() == true
What is surprising in the above expression evaluating to true is the
fact that 'r' appears to be enqueued before and after the q.poll()
returns null. I can easily imagine code that would fail because it never
imagined above expression to evaluate to true. For example:
So r has been enqueued and one poll() removes it, so the second poll()
returns NULL, but r still claims to be enqueued. Sorry I'm not seeing
how that is possible.
David
'r' has been enqueued.
Thread-1:
r.isEnqueued()
q.poll() == null
r.isEnqueued()
Thread-2:
q.poll();
Sequence of actions:
T1: r.isEnqueued() == true
T2: q.poll() executed to the following point (see HERE) and 'r' was the
last element in the queue ('head' has been assigned to null):
Yeah thanks - just realized it is that darned unsynchronized fast-path
again. What a mess.
It a kind of inverse of the original problem.
Original: don't update reference state to enqueued before the queue is
updated
This one: don't update the queue state to empty before the reference
state shows it is de-queued.
So yes the fix here is to move r.queue = null to before the assignment
to head.
Bring on the next race ;-)
Thanks,
David
public Reference? extends T poll() {
if (head == null)
return null;
synchronized (lock) {
return reallyPoll();
}
}
private Reference? extends T reallyPoll() { /* Must hold
lock */
Reference? extends T r = head;
if (r != null) {
head = (r.next == r) ?
null :
r.next; // Unchecked due to the next field having a raw
type in Reference
// HERE
r.queue = NULL;
r.next = r;
queueLength--;
if (r instanceof FinalReference) {
sun.misc.VM.addFinalRefCount(-1);
}
return r;
}
return null;
}
T1: q.poll() finds head == null and returns null;
T1: r.isEnqueued() == true since r.queue is still ENQUEUED
Regards, Peter
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 30/07/15 13:37, Peter Levart wrote: poll() returning null by itself is not surprising, but if 'r' appears to be enqueued before and after the fact, this is surprising. Agreed - not disputing this. The question for me is whether this should be fixed in the same changeset - or whether we should make it in another changeset... It's a different issue, though very similar. Agreed on both counts :-) -- daniel
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 07/30/2015 12:48 PM, Daniel Fuchs wrote: On 30/07/15 12:20, Peter Levart wrote: On 07/30/2015 11:12 AM, Daniel Fuchs wrote: Hi Peter, I'm glad you're looking at this too! We can't have too many eyes :-) On 30/07/15 10:38, Peter Levart wrote: Suppose we have a Reference 'r' and it's associated ReferenceQueue 'q'. Currently it can happen that the following evaluates to true, which is surprising: q.poll() == null r.isEnqueued() But on the other hand this can only happen if two different threads are polling the queue - in which case only one of them will get the reference. In such a situation, the symmetric condition would not be surprising (as the other thread would get q.poll() != null): r.isEnqueued() q.poll() == null The original bug fixed by Kim is more surprising, because there's only one Thread doing the polling, and it does get: r.isEnqueud() q.poll() == null although nobody else is polling the queue. This should no longer occur in this situation with Kim's fix. cheers, -- daniel Yes, these are two different issues. The one Kim has fixed is the race of above expressions with Queue.enqueue() (when the queue is changing from the associated instance to ENQUEUED). The one I'm pointing at and Kim has already identified as potential issue is the race of the following expression: r.isEnqueued() q.poll() == null r.isEnqueued() == true with Queue.[realy]poll() (when the queue is changing from ENQUEUED to NULL). Which only happens if at least two threads are polling the queue, but is equally surprising and prone to cause bugs, don't you think? Hi Peter, Yes - this is also surprising. Is it prone to cause bugs? possibly - but how serious I'm not sure. Is it 'equally' surprising - well - that was the point of my argument: there are two threads polling the same queue - so one of them should get null... Though I agree that in this case - 'r' should be seen has having changed state... poll() returning null by itself is not surprising, but if 'r' appears to be enqueued before and after the fact, this is surprising. The question for me is whether this should be fixed in the same changeset - or whether we should make it in another changeset... It's a different issue, though very similar. Regards, Peter cheers, and thanks for pointing it out! -- daniel Regards, Peter
Re: RfR - 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java
Hi, Please find below an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.01/ Instead of removing the CLITest.java - I copied Process.java into the test hierarchy, renamed it to ProcessXSLT.java, modified it so that it no longer uses internal APIs, and changed the CLITest.java to use that. The only adherence is with com.sun.org.apache.xml.internal.utils.DefaultErrorHandler that the new ProcessXSLT class tries to instantiate through reflection - as that error handler has access to jaxp internals and can give a better error diagnostic. However - the ProcessXSLT will use its own dummy ErrorHandler if it can't managed to instantiate the internal class. This way - we can keep the test :-) best regards, -- daniel On 29/07/15 17:02, Daniel Fuchs wrote: Hi, Please find below a patch that removes a bunch of unused files in jdk9/dev/jaxp: https://bugs.openjdk.java.net/browse/JDK-8130058 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.00/ best regards, -- daniel
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 07/30/2015 01:44 PM, David Holmes wrote:
r.isEnqueued() q.poll() == null r.isEnqueued() == true
What is surprising in the above expression evaluating to true is the
fact that 'r' appears to be enqueued before and after the q.poll()
returns null. I can easily imagine code that would fail because it never
imagined above expression to evaluate to true. For example:
So r has been enqueued and one poll() removes it, so the second poll()
returns NULL, but r still claims to be enqueued. Sorry I'm not seeing
how that is possible.
David
'r' has been enqueued.
Thread-1:
r.isEnqueued()
q.poll() == null
r.isEnqueued()
Thread-2:
q.poll();
Sequence of actions:
T1: r.isEnqueued() == true
T2: q.poll() executed to the following point (see HERE) and 'r' was the
last element in the queue ('head' has been assigned to null):
public Reference? extends T poll() {
if (head == null)
return null;
synchronized (lock) {
return reallyPoll();
}
}
private Reference? extends T reallyPoll() { /* Must hold
lock */
Reference? extends T r = head;
if (r != null) {
head = (r.next == r) ?
null :
r.next; // Unchecked due to the next field having a raw
type in Reference
// HERE
r.queue = NULL;
r.next = r;
queueLength--;
if (r instanceof FinalReference) {
sun.misc.VM.addFinalRefCount(-1);
}
return r;
}
return null;
}
T1: q.poll() finds head == null and returns null;
T1: r.isEnqueued() == true since r.queue is still ENQUEUED
Regards, Peter
RFR Collector.finisher refers to IDENTITY_TRANSFORM rather than INDENTITY_FINISH
Hi,
Please review this simple fix to the JavaDoc on j.u.stream.Collector.finisher.
I am also opportunistically fixing some internal comments identified by Tagir.
Paul.
diff -r 4e3135fac8cc src/java.base/share/classes/java/util/stream/Collector.java
--- a/src/java.base/share/classes/java/util/stream/Collector.java Fri Jul
24 15:33:13 2015 -0700
+++ b/src/java.base/share/classes/java/util/stream/Collector.java Thu Jul
30 17:05:13 2015 +0200
@@ -223,7 +223,7 @@
* Perform the final transformation from the intermediate accumulation type
* {@code A} to the final result type {@code R}.
*
- * pIf the characteristic {@code IDENTITY_TRANSFORM} is
+ * pIf the characteristic {@code IDENTITY_FINISH} is
* set, this function may be presumed to be an identity transform with an
* unchecked cast from {@code A} to {@code R}.
*
diff -r 4e3135fac8cc src/java.base/share/classes/java/util/stream/SliceOps.java
--- a/src/java.base/share/classes/java/util/stream/SliceOps.javaFri Jul
24 15:33:13 2015 -0700
+++ b/src/java.base/share/classes/java/util/stream/SliceOps.javaThu Jul
30 17:05:13 2015 +0200
@@ -138,7 +138,7 @@
skip, limit, size);
}
else {
-// @@@ OOMEs will occur for LongStream.longs().filter(i -
true).limit(n)
+// @@@ OOMEs will occur for LongStream.range(0,
Long.MAX_VALUE)).filter(i - true).limit(n)
// regardless of the value of n
// Need to adjust the target size of splitting for the
// SliceTask from say (size / k) to say min(size / k,
1 14)
diff -r 4e3135fac8cc src/java.base/share/classes/java/util/stream/Streams.java
--- a/src/java.base/share/classes/java/util/stream/Streams.java Fri Jul 24
15:33:13 2015 -0700
+++ b/src/java.base/share/classes/java/util/stream/Streams.java Thu Jul 30
17:05:13 2015 +0200
@@ -156,10 +156,9 @@
* than a balanced tree at the expense of a higher-depth for the right
* side of the range.
*
- * pThis is optimized for cases such as IntStream.ints() that is
- * implemented as range of 0 to Integer.MAX_VALUE but is likely to be
- * augmented with a limit operation that limits the number of elements
- * to a count lower than this threshold.
+ * pThis is optimized for cases such as IntStream.range(0,
Integer.MAX_VALUE)
+ * that is likely to be augmented with a limit operation that limits
the
+ * number of elements to a count lower than this threshold.
*/
private static final int BALANCED_SPLIT_THRESHOLD = 1 24;
@@ -280,10 +279,9 @@
* than a balanced tree at the expense of a higher-depth for the right
* side of the range.
*
- * pThis is optimized for cases such as LongStream.longs() that is
- * implemented as range of 0 to Long.MAX_VALUE but is likely to be
- * augmented with a limit operation that limits the number of elements
- * to a count lower than this threshold.
+ * pThis is optimized for cases such as LongStream.range(0,
Long.MAX_VALUE)
+ * that is likely to be augmented with a limit operation that limits
the
+ * number of elements to a count lower than this threshold.
*/
private static final long BALANCED_SPLIT_THRESHOLD = 1 24;
Re: RFR 9: JDK-8132705 : Refactor SharedSecrets in sun.misc.JavaNetAccess
Looks good Roger. Only minor quibble would be the name of the new type JavaInetAddressAccess could be JavaNetInetAddressAccess to be consistent. Michael On 30/07/15 15:49, Roger Riggs wrote: Please review this refactoring of SharedSecret initialization to create and InetAddressAccess access that is independent of the initialization of JavaNetAccess in URLClassLoader. (jprt in progress) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-inetaccess-8132705/ Issue: https://bugs.openjdk.java.net/browse/JDK-8132705 Roger
Re: RFR 9: JDK-8132705 : Refactor SharedSecrets in sun.misc.JavaNetAccess
Hi, I renamed the new interface and its uses as recommended. The webrev updated in place: http://cr.openjdk.java.net/~rriggs/webrev-inetaccess-8132705/ I deferred renaming JavaNetAccess because it (getURLClassPath) is used in install and deploy and I was not certain it would still be needed with the module system in place. It can be renamed (in a separate changeset) if that's useful. Roger On 7/30/2015 11:08 AM, Alan Bateman wrote: On 30/07/2015 15:49, Roger Riggs wrote: Please review this refactoring of SharedSecret initialization to create and InetAddressAccess access that is independent of the initialization of JavaNetAccess in URLClassLoader. (jprt in progress) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-inetaccess-8132705/ I assume this should JavaNetInetAddressAccess to be consistent. I also wondering if JavaNetAccess should be renamed as it is URLClass* specific. -Alan.
RFR 9: JDK-8132705 : Refactor SharedSecrets in sun.misc.JavaNetAccess
Please review this refactoring of SharedSecret initialization to create and InetAddressAccess access that is independent of the initialization of JavaNetAccess in URLClassLoader. (jprt in progress) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-inetaccess-8132705/ Issue: https://bugs.openjdk.java.net/browse/JDK-8132705 Roger
Re: RFR 9: JDK-8132705 : Refactor SharedSecrets in sun.misc.JavaNetAccess
On 30/07/2015 15:49, Roger Riggs wrote: Please review this refactoring of SharedSecret initialization to create and InetAddressAccess access that is independent of the initialization of JavaNetAccess in URLClassLoader. (jprt in progress) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-inetaccess-8132705/ I assume this should JavaNetInetAddressAccess to be consistent. I also wondering if JavaNetAccess should be renamed as it is URLClass* specific. -Alan.
RFR(S): 8132704: [TESTBUG] jdk/internal/jimage/ExecutableTest.java incorrectly asserts all files to be executable
Hi, can somebody please review this test fix: http://cr.openjdk.java.net/~simonis/webrevs/2015/8132704/ https://bugs.openjdk.java.net/browse/JDK-8132704 The initial test checked that all the files in the bin/ directory are executable by everybody. Unfortunately this was too optimistic because in the closed build the bin/ directory contains configuration files which are not executable. The new version of the test uses a predefined static list of executables which are checked for the executable permissions if the corresponding files exist. Thank you and best regards, Volker
Re: RFR(S): 8132704: [TESTBUG] jdk/internal/jimage/ExecutableTest.java incorrectly asserts all files to be executable
On Thu, Jul 30, 2015 at 5:34 PM, Roger Riggs roger.ri...@oracle.com wrote: Hi Volker, Possibly the real bug is that there is non-executable file in the bin directory. There is a /conf directory which would probably be a better place for that. Yes, I agree. But I thought you want a fast fix for the test failure :) Moving that config file is probably a bigger effort. Moreover the bin/ directory on Windows also contains .dll and .diz files. However on Windows, all the files seem to be executable (at least the test did succeed before). Nevertheless, checking only a known subset of executables seems safer and good enough. What do you think? Volker Roger On 7/30/2015 11:28 AM, Volker Simonis wrote: Hi, can somebody please review this test fix: http://cr.openjdk.java.net/~simonis/webrevs/2015/8132704/ https://bugs.openjdk.java.net/browse/JDK-8132704 The initial test checked that all the files in the bin/ directory are executable by everybody. Unfortunately this was too optimistic because in the closed build the bin/ directory contains configuration files which are not executable. The new version of the test uses a predefined static list of executables which are checked for the executable permissions if the corresponding files exist. Thank you and best regards, Volker
Re: RFR 9: JDK-8132705 : Refactor SharedSecrets in sun.misc.JavaNetAccess
Thanks for doing this Roger. The changes look good to me. -Chris. On 30 Jul 2015, at 15:49, Roger Riggs roger.ri...@oracle.com wrote: Please review this refactoring of SharedSecret initialization to create and InetAddressAccess access that is independent of the initialization of JavaNetAccess in URLClassLoader. (jprt in progress) Webrev: http://cr.openjdk.java.net/~rriggs/webrev-inetaccess-8132705/ Issue: https://bugs.openjdk.java.net/browse/JDK-8132705 Roger
Re: RFR(S): 8132704: [TESTBUG] jdk/internal/jimage/ExecutableTest.java incorrectly asserts all files to be executable
Hi Volker, Possibly the real bug is that there is non-executable file in the bin directory. There is a /conf directory which would probably be a better place for that. Roger On 7/30/2015 11:28 AM, Volker Simonis wrote: Hi, can somebody please review this test fix: http://cr.openjdk.java.net/~simonis/webrevs/2015/8132704/ https://bugs.openjdk.java.net/browse/JDK-8132704 The initial test checked that all the files in the bin/ directory are executable by everybody. Unfortunately this was too optimistic because in the closed build the bin/ directory contains configuration files which are not executable. The new version of the test uses a predefined static list of executables which are checked for the executable permissions if the corresponding files exist. Thank you and best regards, Volker
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 31/07/2015 5:54 AM, Kim Barrett wrote:
On Jul 30, 2015, at 8:09 AM, David Holmes david.hol...@oracle.com wrote:
On 30/07/2015 9:57 PM, Peter Levart wrote:
'r' has been enqueued.
Thread-1:
r.isEnqueued()
q.poll() == null
r.isEnqueued()
Thread-2:
q.poll();
Sequence of actions:
T1: r.isEnqueued() == true
T2: q.poll() executed to the following point (see HERE) and 'r' was the
last element in the queue ('head' has been assigned to null):
Yeah thanks - just realized it is that darned unsynchronized fast-path again.
What a mess.
It a kind of inverse of the original problem.
Original: don't update reference state to enqueued before the queue is updated
This one: don't update the queue state to empty before the reference state
shows it is de-queued.
So yes the fix here is to move r.queue = null to before the assignment to
head.
Bring on the next race ;-)
I agree with everything David said above. Bleh!
So I think I can either:
1. Go ahead with my change + Peter's change.
2. Give this back to core-libs while I step carefully away :-)
I *think* option (1) is at least an improvement. But I completely
missed Peter's race, despite having specifically looked for problems
there, so take my opinion with an appropriate quantity of salt.
I vote for 1 as well. I think we have now given good coverage to the two
sources of problems (the two lock-free regions):
- reference queue state can be seen while queue state is in transition
- queue empty state can be seen while reference state is in transition
Oh for a tool that would do this analysis for us :(
Thanks,
David
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On Jul 30, 2015, at 8:09 AM, David Holmes david.hol...@oracle.com wrote:
On 30/07/2015 9:57 PM, Peter Levart wrote:
'r' has been enqueued.
Thread-1:
r.isEnqueued()
q.poll() == null
r.isEnqueued()
Thread-2:
q.poll();
Sequence of actions:
T1: r.isEnqueued() == true
T2: q.poll() executed to the following point (see HERE) and 'r' was the
last element in the queue ('head' has been assigned to null):
Yeah thanks - just realized it is that darned unsynchronized fast-path
again. What a mess.
It a kind of inverse of the original problem.
Original: don't update reference state to enqueued before the queue is updated
This one: don't update the queue state to empty before the reference state
shows it is de-queued.
So yes the fix here is to move r.queue = null to before the assignment to
head.
Bring on the next race ;-)
I agree with everything David said above. Bleh!
So I think I can either:
1. Go ahead with my change + Peter's change.
2. Give this back to core-libs while I step carefully away :-)
I *think* option (1) is at least an improvement. But I completely
missed Peter's race, despite having specifically looked for problems
there, so take my opinion with an appropriate quantity of salt.
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
I vote for 1.
:-)
cheers,
-- daniel
On 7/30/15 9:54 PM, Kim Barrett wrote:
On Jul 30, 2015, at 8:09 AM, David Holmes david.hol...@oracle.com wrote:
On 30/07/2015 9:57 PM, Peter Levart wrote:
'r' has been enqueued.
Thread-1:
r.isEnqueued()
q.poll() == null
r.isEnqueued()
Thread-2:
q.poll();
Sequence of actions:
T1: r.isEnqueued() == true
T2: q.poll() executed to the following point (see HERE) and 'r' was the
last element in the queue ('head' has been assigned to null):
Yeah thanks - just realized it is that darned unsynchronized fast-path again.
What a mess.
It a kind of inverse of the original problem.
Original: don't update reference state to enqueued before the queue is updated
This one: don't update the queue state to empty before the reference state
shows it is de-queued.
So yes the fix here is to move r.queue = null to before the assignment to
head.
Bring on the next race ;-)
I agree with everything David said above. Bleh!
So I think I can either:
1. Go ahead with my change + Peter's change.
2. Give this back to core-libs while I step carefully away :-)
I *think* option (1) is at least an improvement. But I completely
missed Peter's race, despite having specifically looked for problems
there, so take my opinion with an appropriate quantity of salt.
Re: RFR: 8132459: ExceptionInInitializerError from 'java -version' on Linux under zh_CN.GB18030 locale
On 07/30/2015 03:01 PM, David Holmes wrote: On 31/07/2015 1:41 AM, Xueming Shen wrote: On 07/30/2015 01:37 AM, Volker Simonis wrote: On Thu, Jul 30, 2015 at 9:51 AM, Alan Batemanalan.bate...@oracle.com wrote: On 30/07/2015 06:21, Xueming Shen wrote: : Each platform has a list of supported locale/encoding. All these encodings/charsets need to be in base module for that particular platform, to support the jvm to start (in a particular locale/encoding) under module system. The charsets in our repository can be categorized into different groups, solaris/ linux specific, windows specific and IBM specific and couple that are shared by different platforms). The idea here is to build all those platform-specific charsets into the base module for that platform. Right, and furthermore, we should be able to build a compact1 image or just an image with the java.base module and it should be able to start on platforms when running with a supported locale/encoding. I think the main issue we've had is establishing that list, hence it had to be extended a few times. The change looks fine. But what about the 'supported locale/encoding' list. Is there a published 'official' version of this list for Oracle/OpenJDK and how is it maintained. I meant to say all the supported/native locale+encoding of the platform/OS. If we have those charsets in our repository, they all need go into the base module. Now I'm confused again. Do the platforms we officially support have set lists of such locales/encodings? If so getting our list correct seems trivial. If not, then how can we support an unknown target?? it's a known target. we know the list of the locale/encoding solais /linux supports and the list of charsets Java supports. -sherman
Re: RFR(xs): 8132745: minor cleanup of java/util/Scanner/ScanTest.java
Looks fine Stuart, -Joe On 7/30/2015 3:39 PM, Stuart Marks wrote: Hi all, Please review this quick cleanup to this test I moved into the open yesterday. It changes the JVM's locale, and out of an abundance of caution it's safer to run things that change JVM global state in /othervm mode. Bug: https://bugs.openjdk.java.net/browse/JDK-8132745 Patch appended below. Thanks, s'marks # HG changeset patch # User smarks # Date 1438295744 25200 # Thu Jul 30 15:35:44 2015 -0700 # Node ID 4ce7979b7610574075bd4ade3b73004cde5e9ac3 # Parent d23203801b5369603d2dda21a6aff6225195001d 8132745: minor cleanup of java/util/Scanner/ScanTest.java Reviewed-by: XXX diff -r d23203801b53 -r 4ce7979b7610 test/java/util/Scanner/ScanTest.java --- a/test/java/util/Scanner/ScanTest.javaThu Jul 30 14:16:58 2015 -0400 +++ b/test/java/util/Scanner/ScanTest.javaThu Jul 30 15:35:44 2015 -0700 @@ -26,6 +26,7 @@ * @bug 4313885 4926319 4927634 5032610 5032622 5049968 5059533 6223711 6277261 6269946 6288823 * @summary Basic tests of java.util.Scanner methods * @key randomness + * @run main/othervm ScanTest */ import java.util.*;
Re: RFR(xs): 8132745: minor cleanup of java/util/Scanner/ScanTest.java
+1 On 07/30/2015 03:40 PM, Joseph D. Darcy wrote: Looks fine Stuart, -Joe On 7/30/2015 3:39 PM, Stuart Marks wrote: Hi all, Please review this quick cleanup to this test I moved into the open yesterday. It changes the JVM's locale, and out of an abundance of caution it's safer to run things that change JVM global state in /othervm mode. Bug: https://bugs.openjdk.java.net/browse/JDK-8132745 Patch appended below. Thanks, s'marks # HG changeset patch # User smarks # Date 1438295744 25200 # Thu Jul 30 15:35:44 2015 -0700 # Node ID 4ce7979b7610574075bd4ade3b73004cde5e9ac3 # Parent d23203801b5369603d2dda21a6aff6225195001d 8132745: minor cleanup of java/util/Scanner/ScanTest.java Reviewed-by: XXX diff -r d23203801b53 -r 4ce7979b7610 test/java/util/Scanner/ScanTest.java --- a/test/java/util/Scanner/ScanTest.javaThu Jul 30 14:16:58 2015 -0400 +++ b/test/java/util/Scanner/ScanTest.javaThu Jul 30 15:35:44 2015 -0700 @@ -26,6 +26,7 @@ * @bug 4313885 4926319 4927634 5032610 5032622 5049968 5059533 6223711 6277261 6269946 6288823 * @summary Basic tests of java.util.Scanner methods * @key randomness + * @run main/othervm ScanTest */ import java.util.*;
Re: RFR: 8132459: ExceptionInInitializerError from 'java -version' on Linux under zh_CN.GB18030 locale
Hello, I doubt you can compile a list of all charsets supported by the various (national) linux distributions and versions (and installed runtime packages). Especially not for OpenJDK which can be much less restrictive in picking supported (non enterprise) distributions. And I also wonder why it is needed, if a locale is not known just fall back to a sane default. The OpenJDK should define a reasonable list (especially for compact profiles) and not declare to support all platforms fully. Gruss Bernd Xueming Shen xueming.s...@oracle.com schrieb am Fr., 31. Juli 2015 00:35: On 07/30/2015 03:01 PM, David Holmes wrote: On 31/07/2015 1:41 AM, Xueming Shen wrote: On 07/30/2015 01:37 AM, Volker Simonis wrote: On Thu, Jul 30, 2015 at 9:51 AM, Alan Batemanalan.bate...@oracle.com wrote: On 30/07/2015 06:21, Xueming Shen wrote: : Each platform has a list of supported locale/encoding. All these encodings/charsets need to be in base module for that particular platform, to support the jvm to start (in a particular locale/encoding) under module system. The charsets in our repository can be categorized into different groups, solaris/ linux specific, windows specific and IBM specific and couple that are shared by different platforms). The idea here is to build all those platform-specific charsets into the base module for that platform. Right, and furthermore, we should be able to build a compact1 image or just an image with the java.base module and it should be able to start on platforms when running with a supported locale/encoding. I think the main issue we've had is establishing that list, hence it had to be extended a few times. The change looks fine. But what about the 'supported locale/encoding' list. Is there a published 'official' version of this list for Oracle/OpenJDK and how is it maintained. I meant to say all the supported/native locale+encoding of the platform/OS. If we have those charsets in our repository, they all need go into the base module. Now I'm confused again. Do the platforms we officially support have set lists of such locales/encodings? If so getting our list correct seems trivial. If not, then how can we support an unknown target?? it's a known target. we know the list of the locale/encoding solais /linux supports and the list of charsets Java supports. -sherman
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On Jul 30, 2015, at 5:33 PM, David Holmes david.hol...@oracle.com wrote: So I think I can either: 1. Go ahead with my change + Peter's change. 2. Give this back to core-libs while I step carefully away :-) I *think* option (1) is at least an improvement. But I completely missed Peter's race, despite having specifically looked for problems there, so take my opinion with an appropriate quantity of salt. I vote for 1 as well. I think we have now given good coverage to the two sources of problems (the two lock-free regions): - reference queue state can be seen while queue state is in transition - queue empty state can be seen while reference state is in transition New webrev, with both changes: http://cr.openjdk.java.net/~kbarrett/8132306/webrev.01/
Re: RFR (M/L): 8131168: Refactor ProcessHandleImpl_*.c and add implememtation for AIX
On 7/29/15 11:36 AM, Volker Simonis wrote: !! ProcessHandleImpl_unix: 709-738: I still disagree with returning truncated or incomplete values for the executable or arguments. Can anyone be a tie-breaker (with a good rational and suggestion for how an application can use the data). As I wrote, I agree to hear other opinions here. All I want to say that this truncated data is actually what 'ps' is reporting on Solaris since decades and people seem to be happy with it. As use case I can imagine logging or monitoring (something like 'top' in Java) where this data will be just good enough. We could specially mark possibly incomplete data by extending the Info object with functions like commandIsExact() or argumentsIsPrecise(). But notice that we can not statically preset these values per platform. For example on Solaris, the 'command()' would return a precise value for processes with the same uid like the VM but only inaccurate values for all other processes. The arguments() would be always inaccurate on Solaris/AIX. It seems like there are cases where either exact or only approximate information is available. And as you observed, you might get one or the other on the same platform, depending on the UID. It also might depend on process state; I believe that some information becomes inaccessible when the process enters the zombie state. I don't think we should simply ignore one case or the other, but I also don't think we should try to cram the different information into the same API. The current ProcessHandle.Info api has OptionalString command() OptionalString[] arguments() It sounds to me like Roger wants these to contain only exact information. That seems reasonable, and this probably needs to be specified more clearly to contrast with what's below. On Solaris, the psinfo_t struct has char pr_psargs[PRARGSZ] which is a single string which appears to be the concatenation of the arguments (maybe including the command name). It's also truncated to fit PRARGSZ. It doesn't make sense to me to try to return this as a String[], as the zeroth element of that array, and certainly not parsed out into words. So maybe instead we should have a different API that returns an imprecise command line as a single string: OptionalString cmdline() (Naming bikeshed TBS). The semantics would be that this is the process' command and arguments concatenated into a single string (thus potentially losing argument boundaries) and also possibly truncated based on platform (COUGHsolarisCOUGH) limitations. It's certainly useful for printing out in a ps, top, or Activity Monitor style application, for informational purposes. If this were implemented, then on Solaris, command() and arguments() would always return empty optionals. I'm not sure what this should be if the exact information is available. It would be inconvenient if something that just wanted to print out an approximate command line had to check several different APIs to get the information. Maybe cmdline() could assemble the information from exact data if it's is available, by concatenating the Strings from command() and arguments(), as a convenience to the caller. But I could go either way on this. Not sure this counts as a tie-breaker, but it might be a reasonable way forward. s'marks might be convenient if this
Re: RFR: 8132459: ExceptionInInitializerError from 'java -version' on Linux under zh_CN.GB18030 locale
On 31/07/2015 1:41 AM, Xueming Shen wrote: On 07/30/2015 01:37 AM, Volker Simonis wrote: On Thu, Jul 30, 2015 at 9:51 AM, Alan Batemanalan.bate...@oracle.com wrote: On 30/07/2015 06:21, Xueming Shen wrote: : Each platform has a list of supported locale/encoding. All these encodings/charsets need to be in base module for that particular platform, to support the jvm to start (in a particular locale/encoding) under module system. The charsets in our repository can be categorized into different groups, solaris/ linux specific, windows specific and IBM specific and couple that are shared by different platforms). The idea here is to build all those platform-specific charsets into the base module for that platform. Right, and furthermore, we should be able to build a compact1 image or just an image with the java.base module and it should be able to start on platforms when running with a supported locale/encoding. I think the main issue we've had is establishing that list, hence it had to be extended a few times. The change looks fine. But what about the 'supported locale/encoding' list. Is there a published 'official' version of this list for Oracle/OpenJDK and how is it maintained. I meant to say all the supported/native locale+encoding of the platform/OS. If we have those charsets in our repository, they all need go into the base module. Now I'm confused again. Do the platforms we officially support have set lists of such locales/encodings? If so getting our list correct seems trivial. If not, then how can we support an unknown target?? Thanks, David We do have a jdk supported locale and encoding list, but they are for the supported java locale and encoding. -Sherman Regards, Volker -Alan.
Re: RFR 8130828: Fix some typos and omissions in the the j.u.stream JavaDoc
Hi Paul, Changes look good. (The webrev changesets are a bit odd, though; are you running webrev on a branchy repo or something?) Stefan, Tagir, thanks for spotting these issues. s'marks On 7/30/15 9:32 AM, Paul Sandoz wrote: Hi Stefan, Tagir, Updated: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8130828-stream-doc-typos/webrev/ Paul. On 30 Jul 2015, at 18:08, Stefan Zobel splitera...@gmail.com wrote: Hi Paul, perhaps you could take the opportunity and also add the missing @since 1.9 tags to all the new dropWhile() / takeWhile() methods (JDK-8071597). Also in dropWhile() / takeWhile() there is a small typo (I guess) in the Javadoc: takeWhile: takes all elements (the result is the same is the input) dropWhile: the stream match the given predicate then no elements are dropped (the result is the same is the input) I guess that should read: (the result is the same as is the input)? Stefan
Re: RFR JDK-8080252: java.util.Formatter documentation of %n converter is misleading
Hi Sherman, The change looks fine to me. s'marks On 7/30/15 10:34 AM, Xueming Shen wrote: Hi, Please help review the change for issue: https://bugs.openjdk.java.net/browse/JDK-8080252 webrev: http://cr.openjdk.java.net/~sherman/8080252/ It appears we updated the j.u.Formatter implementation to use the newly introduced method System.lineSeparator() in jdk7, but did not update the doc accordingly. For the old the behavior, I would assume the initial intent is to treat the system property line.separator as a read-only/informative property. Given the fact the it has been 2 major releases (7, 8), I would assume this change is now a kind of change to update the javadoc to describe the existing implementation correctly. So a CCC is not needed. Thanks, Sherman
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
Looks good! Thanks, David On 31/07/2015 8:56 AM, Kim Barrett wrote: On Jul 30, 2015, at 5:33 PM, David Holmes david.hol...@oracle.com wrote: So I think I can either: 1. Go ahead with my change + Peter's change. 2. Give this back to core-libs while I step carefully away :-) I *think* option (1) is at least an improvement. But I completely missed Peter's race, despite having specifically looked for problems there, so take my opinion with an appropriate quantity of salt. I vote for 1 as well. I think we have now given good coverage to the two sources of problems (the two lock-free regions): - reference queue state can be seen while queue state is in transition - queue empty state can be seen while reference state is in transition New webrev, with both changes: http://cr.openjdk.java.net/~kbarrett/8132306/webrev.01/
Re: RFR: JDK-8114832 it.next on ArrayList throws wrong type of Exception after remove(-1)
On 7/28/15 5:50 AM, Paul Sandoz wrote: I agree that if we make a policy decision here, we can change it and the compatibility impact is minimal. Since there already exists an implicit policy governed by like 99.9% of the existing behaviour i do not see the need to be explicit about that policy for this particular issue, so i suggest we split into two. I followed this thread up until the last sentence. What's the resolution here? s'marks
Re: RFR: 8132459: ExceptionInInitializerError from 'java -version' on Linux under zh_CN.GB18030 locale
On 7/30/15 4:55 PM, Bernd wrote: Hello, I doubt you can compile a list of all charsets supported by the various (national) linux distributions and versions (and installed runtime packages). Especially not for OpenJDK which can be much less restrictive in picking supported (non enterprise) distributions. And I also wonder why it is needed, if a locale is not known just fall back to a sane default. The OpenJDK should define a reasonable list (especially for compact profiles) and not declare to support all platforms fully. Bernd, it's the other way around. Instead of addding all charsets supported by the various linux distributions/versions into the base module, we are selecting/building all charsets from our existing repository (originally in jdk's standard charsets and extended charsets) that might be used for a particular platform (linux, solaris, macos, windows) into the base module for that platform. The configuration is pretty straightforward, you can easily define a reasonable list of charsets and build the OpenJDK for your specified platform/profile. As discussed in other email, we are working on the best approach for the use scenario that a charset is unsupported from our repository. -sherman Xueming Shen xueming.s...@oracle.com mailto:xueming.s...@oracle.com schrieb am Fr., 31. Juli 2015 00:35: On 07/30/2015 03:01 PM, David Holmes wrote: On 31/07/2015 1:41 AM, Xueming Shen wrote: On 07/30/2015 01:37 AM, Volker Simonis wrote: On Thu, Jul 30, 2015 at 9:51 AM, Alan Batemanalan.bate...@oracle.com mailto:alan.bate...@oracle.com wrote: On 30/07/2015 06:21, Xueming Shen wrote: : Each platform has a list of supported locale/encoding. All these encodings/charsets need to be in base module for that particular platform, to support the jvm to start (in a particular locale/encoding) under module system. The charsets in our repository can be categorized into different groups, solaris/ linux specific, windows specific and IBM specific and couple that are shared by different platforms). The idea here is to build all those platform-specific charsets into the base module for that platform. Right, and furthermore, we should be able to build a compact1 image or just an image with the java.base module and it should be able to start on platforms when running with a supported locale/encoding. I think the main issue we've had is establishing that list, hence it had to be extended a few times. The change looks fine. But what about the 'supported locale/encoding' list. Is there a published 'official' version of this list for Oracle/OpenJDK and how is it maintained. I meant to say all the supported/native locale+encoding of the platform/OS. If we have those charsets in our repository, they all need go into the base module. Now I'm confused again. Do the platforms we officially support have set lists of such locales/encodings? If so getting our list correct seems trivial. If not, then how can we support an unknown target?? it's a known target. we know the list of the locale/encoding solais /linux supports and the list of charsets Java supports. -sherman
Re: RFR: 8132459: ExceptionInInitializerError from 'java -version' on Linux under zh_CN.GB18030 locale
On 30/07/2015 04:54, Xueming Shen wrote: Here is the webrev to add those missing charsets. The assumption back then was that the linux platform has successfully migrated to the utf-8 default world. http://cr.openjdk.java.net/~sherman/8132459/ This looks okay to me. -Alan
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
Hi,
Let me chime in and add some comments...
On 07/30/2015 01:46 AM, Kim Barrett wrote:
On Jul 29, 2015, at 4:32 AM, David Holmes david.hol...@oracle.com wrote:
On 29/07/2015 5:57 PM, Kim Barrett wrote:
... The race is being fixed by reordering a pair
of volatile assignments.
While this seems logical for the failure at hand it isn't immediately obvious
to me that setting next before setting the ENQUEUED state won't cause a problem
for some other piece of code. Really these things need to be tightly
synchronized - I think the real bug is the unsynchronized fast-path for the
empty-queue case in poll(). While that change was deliberate in 739 this
side-effect was not realized and would have affected the change. I hope Martin
and/or Tom see this and chime in.
I thought the poll() fast-path from 739 was ok at the time it was
made, and that it was the later removal of synchronization on the
reference by 8014890 that lead to the race under discussion, and
reinstating that synchronization on the reference was another way to
fix this race. Turns out I was wrong about that.
The poll() fast-path introduced the possibility of the following
unexpected behavior when another thread is in r.enqueue() and is in
the problematic window:
!r.enqueue() q.poll() == null = true
This can happen because the synchronization on the references is only
in ReferenceQueue.enqueue(). If it was instead in
Reference.enqueue(), or if ReferenceQueue.Null.enqueue() also
synchronized on the reference, this race would be prevented.
Isn't above condition perfectly legal for references that have already
been de-queued (and when the 'q' is otherwise empty)? But I see what you
mean. If r.enqueue() grabs the ENQUEUED queue, the method returns false,
but that does not mean that the reference has already been hooked on the
'head' chain ('head' can still be null and q.poll() would return null
because of fast-path).
reversing assignments of 'head' and 'queue' fix this situation too.
The removal of reference synchronization opened the door wider, also
allowing this unexpected behavior:
r.isEnqueued() q.poll() == null = true
This condition is more representative, yes...
The combination of those changes is needed for the ReferenceEnqueue
regression test to fail, since it requires
r.isEnqueued() !r.enqueue() q.poll() == null = true
I wouldn't want to revert the poll() fast-path, since that was added
for specific performance reasons.
I don't think I'd want to add back synchronization on the reference,
but might be persuaded otherwise. 8029205 should be looked at in that
case.
I've looked carefully at uses of r.next and I don't think there is a
problem with reordering the r.next and r.queue assignments. Of
course, the existing code was looked at by a number of people without
spotting the race under discussion.
A way to think about this that helped me (I think) understand the
locking structure used here is that q.head and r.next are part of the
queue associated with the reference. We can manipulate those using
the queue's lock as the basis for protection, so long as the the
reference's queue isn't changed. But when we change the reference's
queue, the queue (including r.next) must be in a consistent state.
[This suggests the r.queue = NULL assignment in reallyPoll() should be
moved later, though I think the assignment order in reallyPoll()
doesn't matter.]
I think the assignment to r.queue = NULL in realyPoll() should be moved
*before* the assignment to 'head' (which might assign null if 'r' was
the last element). Here's why:
Suppose we have a Reference 'r' and it's associated ReferenceQueue 'q'.
Currently it can happen that the following evaluates to true, which is
surprising:
q.poll() == null r.isEnqueued()
Regards, Peter
That aside the commentary is rather verbose, a simple:
// Only set ENQUEUED state after the reference is enqueued
would suffice (and add volatiles ensure ordering if you want that to be
clearer).
I do get a bit wordy sometimes. How about this:
// Update r.queue *after* adding to queue's list, to avoid
// race between enqueued checks and poll(). Volatiles
// ensure ordering.
CR:
https://bugs.openjdk.java.net/browse/JDK-8132306
Webrev:
http://cr.openjdk.java.net/~kbarrett/8132306/webrev.00/
Testing:
jprt with default and hotspot testsets
Locally tested race in original code by insertion of sleep in enqueue and
verifying ReferenceEnqueue regression test fails consistently as
described in the bug report.
Locally tested fixed code by insertion of sleeps at various points and
verifying ReferenceEnqueue regression test still passes.
Re: Custom spliterator for Collections.nCopies(n, obj).stream()
On 30 Jul 2015, at 08:08, Tagir F. Valeev amae...@gmail.com wrote:
Hello!
PS I don’t particular want to add a special spliterator for this
PS case to avoid some profile pollution. Will it not just push the
PS pollution further down the road to Spliterator.forEachRemaining? or to
within other code?
I just thought that the current idea is to create specialized
spliterators for most methods returning streams.
Not without some evaluation of the benefits compared to the increased cost of
code.
If not, why
String.chars()/AbstractStringBuilder.chars() in JDK9 use new
IntCharArraySpliterator instead of already existing
CharBuffer.wrap(this).chars() which produce similar performance in
both sequential and parallel cases? Also for String an alternative
would be
return IntStream.range(0, value.length).map(i - value[i]);
Since String is a commonly used class i opted for the most efficient solution
(both for characters and code points, and shared across String and the
builders).
Which is actually similar to Collections.nCopies().stream().
Yes, but i doubt it is as commonly used as String and thus I don’t think the
argument of profile pollution is sufficient reason to justify a specific
implementation. In this case convenience won over more code.
Also note that Collections class already contains singletonSpliterator
which creates an anonymous class. With my proposed change it can be
replaced with new ConstantSpliterator(1, element) without performance
drop, so actual number of classes will not increase.
With reuse it becomes more compelling :-) In both cases of singleton/nCopies
the spliterator characteristics can be the same and that of the already
existing singleton spliterator implementation.
I would be happy to accept a patch (with tests, if existing tests do not cover
this already, i suspect they might but we still need to check). Have you signed
the OCA [1]. If so i can accept a patch from you and publish as a webrev for
review.
At very least why creating two distinct lambdas in CopiesList.stream()
and CopiesList.parallelStream()? They duplicate i - element, for
which javac creates two separate methods (like lambda$stream$95(int)
and lambda$parallelStream$96(int)) and in runtime two distinct
anonymous classes may be created. It could be written instead
public StreamE parallelStream() {
return stream().parallel();
}
Yes, good point.
Thanks,
Paul.
[1] http://www.oracle.com/technetwork/community/oca-486395.html
Re: RFR: 8132459: ExceptionInInitializerError from 'java -version' on Linux under zh_CN.GB18030 locale
On 30/07/2015 06:21, Xueming Shen wrote: : Each platform has a list of supported locale/encoding. All these encodings/charsets need to be in base module for that particular platform, to support the jvm to start (in a particular locale/encoding) under module system. The charsets in our repository can be categorized into different groups, solaris/ linux specific, windows specific and IBM specific and couple that are shared by different platforms). The idea here is to build all those platform-specific charsets into the base module for that platform. Right, and furthermore, we should be able to build a compact1 image or just an image with the java.base module and it should be able to start on platforms when running with a supported locale/encoding. I think the main issue we've had is establishing that list, hence it had to be extended a few times. -Alan.
Re: RFR: 8132459: ExceptionInInitializerError from 'java -version' on Linux under zh_CN.GB18030 locale
On Thu, Jul 30, 2015 at 9:51 AM, Alan Bateman alan.bate...@oracle.com wrote: On 30/07/2015 06:21, Xueming Shen wrote: : Each platform has a list of supported locale/encoding. All these encodings/charsets need to be in base module for that particular platform, to support the jvm to start (in a particular locale/encoding) under module system. The charsets in our repository can be categorized into different groups, solaris/ linux specific, windows specific and IBM specific and couple that are shared by different platforms). The idea here is to build all those platform-specific charsets into the base module for that platform. Right, and furthermore, we should be able to build a compact1 image or just an image with the java.base module and it should be able to start on platforms when running with a supported locale/encoding. I think the main issue we've had is establishing that list, hence it had to be extended a few times. The change looks fine. But what about the 'supported locale/encoding' list. Is there a published 'official' version of this list for Oracle/OpenJDK and how is it maintained. Regards, Volker -Alan.
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 07/30/2015 10:38 AM, Peter Levart wrote: [This suggests the r.queue = NULL assignment in reallyPoll() should be moved later, though I think the assignment order in reallyPoll() doesn't matter.] I think the assignment to r.queue = NULL in realyPoll() should be moved *before* the assignment to 'head' (which might assign null if 'r' was the last element). Here's why: Suppose we have a Reference 'r' and it's associated ReferenceQueue 'q'. Currently it can happen that the following evaluates to true, which is surprising: q.poll() == null r.isEnqueued() Regards, Peter Well, the above condition is not very representative of the situation I'm trying to describe. It can be perfectly legal when racing with enqueueing of the Reference 'r'. But when racing with de-queueing (i.e. poll()) it is surprising. So let me re-phrase the expression which is always surprising when it evaluates to true: r.isEnqueued() q.poll() == null r.isEnqueued() Regards, Peter
Re: RFR(S): 8132704: [TESTBUG] jdk/internal/jimage/ExecutableTest.java incorrectly asserts all files to be executable
Hi Volker, There is already as task for JMC to move that file but who knows exactly when. A list of exception(s) would be better I think, because generally, everything in bin should be executable. WDYT? Roger On 7/30/2015 11:40 AM, Volker Simonis wrote: On Thu, Jul 30, 2015 at 5:34 PM, Roger Riggs roger.ri...@oracle.com wrote: Hi Volker, Possibly the real bug is that there is non-executable file in the bin directory. There is a /conf directory which would probably be a better place for that. Yes, I agree. But I thought you want a fast fix for the test failure :) Moving that config file is probably a bigger effort. Moreover the bin/ directory on Windows also contains .dll and .diz files. However on Windows, all the files seem to be executable (at least the test did succeed before). Nevertheless, checking only a known subset of executables seems safer and good enough. What do you think? Volker Roger On 7/30/2015 11:28 AM, Volker Simonis wrote: Hi, can somebody please review this test fix: http://cr.openjdk.java.net/~simonis/webrevs/2015/8132704/ https://bugs.openjdk.java.net/browse/JDK-8132704 The initial test checked that all the files in the bin/ directory are executable by everybody. Unfortunately this was too optimistic because in the closed build the bin/ directory contains configuration files which are not executable. The new version of the test uses a predefined static list of executables which are checked for the executable permissions if the corresponding files exist. Thank you and best regards, Volker
Re: RfR - 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java
Hi Daniel, On 7/30/2015 6:38 AM, Daniel Fuchs wrote: Hi, Please find below an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.01/ Looks good to me. The only nit is it seems createDefaultErrorHandler method is a dup of createDefaultErrorListener? Instead of removing the CLITest.java - I copied Process.java into the test hierarchy, renamed it to ProcessXSLT.java, modified it so that it no longer uses internal APIs, and changed the CLITest.java to use that. This is a good idea as it keeps the Process utility around. The only adherence is with com.sun.org.apache.xml.internal.utils.DefaultErrorHandler that the new ProcessXSLT class tries to instantiate through reflection - as that error handler has access to jaxp internals and can give a better error diagnostic. However - the ProcessXSLT will use its own dummy ErrorHandler if it can't managed to instantiate the internal class. This way - we can keep the test :-) Perfect indeed, and if others (Yuri) want to use it, it's readily usable. best, Joe best regards, -- daniel On 29/07/15 17:02, Daniel Fuchs wrote: Hi, Please find below a patch that removes a bunch of unused files in jdk9/dev/jaxp: https://bugs.openjdk.java.net/browse/JDK-8130058 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.00/ best regards, -- daniel
Re: RFR Collector.finisher refers to IDENTITY_TRANSFORM rather than INDENTITY_FINISH
Hello!
Seems that you have an extra parenthesis here:
diff -r 4e3135fac8cc src/java.base/share/classes/java/util/stream/SliceOps.java
--- a/src/java.base/share/classes/java/util/stream/SliceOps.javaFri Jul
24 15:33:13 2015 -0700
+++ b/src/java.base/share/classes/java/util/stream/SliceOps.javaThu Jul
30 17:05:13 2015 +0200
@@ -138,7 +138,7 @@
skip, limit, size);
}
else {
-// @@@ OOMEs will occur for LongStream.longs().filter(i -
true).limit(n)
+// @@@ OOMEs will occur for LongStream.range(0,
Long.MAX_VALUE)).filter(i - true).limit(n)
Should be
+// @@@ OOMEs will occur for LongStream.range(0,
Long.MAX_VALUE).filter(i - true).limit(n)
With best regards,
Tagir Valeev.
Re: RfR - 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java
On 30/07/15 18:16, huizhe wang wrote: On 7/30/2015 9:08 AM, Daniel Fuchs wrote: On 30/07/15 17:55, huizhe wang wrote: Hi Daniel, On 7/30/2015 6:38 AM, Daniel Fuchs wrote: Hi, Please find below an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.01/ Looks good to me. The only nit is it seems createDefaultErrorHandler method is a dup of createDefaultErrorListener? Yes it is - but the return type is different. Yes, but didn't see it's used. Good point! Removed. -- daniel Joe Instead of removing the CLITest.java - I copied Process.java into the test hierarchy, renamed it to ProcessXSLT.java, modified it so that it no longer uses internal APIs, and changed the CLITest.java to use that. This is a good idea as it keeps the Process utility around. The only adherence is with com.sun.org.apache.xml.internal.utils.DefaultErrorHandler that the new ProcessXSLT class tries to instantiate through reflection - as that error handler has access to jaxp internals and can give a better error diagnostic. However - the ProcessXSLT will use its own dummy ErrorHandler if it can't managed to instantiate the internal class. This way - we can keep the test :-) Perfect indeed, and if others (Yuri) want to use it, it's readily usable. Thanks Joe! -- daniel best, Joe best regards, -- daniel On 29/07/15 17:02, Daniel Fuchs wrote: Hi, Please find below a patch that removes a bunch of unused files in jdk9/dev/jaxp: https://bugs.openjdk.java.net/browse/JDK-8130058 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.00/ best regards, -- daniel
Re: Custom spliterator for Collections.nCopies(n, obj).stream()
Hello!
PS I don’t particular want to add a special spliterator for this
PS case to avoid some profile pollution. Will it not just push the
PS pollution further down the road to Spliterator.forEachRemaining? or to
within other code?
I just thought that the current idea is to create specialized
spliterators for most methods returning streams. If not, why
String.chars()/AbstractStringBuilder.chars() in JDK9 use new
IntCharArraySpliterator instead of already existing
CharBuffer.wrap(this).chars() which produce similar performance in
both sequential and parallel cases? Also for String an alternative
would be
return IntStream.range(0, value.length).map(i - value[i]);
Which is actually similar to Collections.nCopies().stream().
Also note that Collections class already contains singletonSpliterator
which creates an anonymous class. With my proposed change it can be
replaced with new ConstantSpliterator(1, element) without performance
drop, so actual number of classes will not increase.
At very least why creating two distinct lambdas in CopiesList.stream()
and CopiesList.parallelStream()? They duplicate i - element, for
which javac creates two separate methods (like lambda$stream$95(int)
and lambda$parallelStream$96(int)) and in runtime two distinct
anonymous classes may be created. It could be written instead
public StreamE parallelStream() {
return stream().parallel();
}
With best regards,
Tagir Valeev.
PS Alas i think profile pollution is current fact of JDK life when
PS inverting control with lambdas. What we really require is a better way to
specialise the hot loops.
PS Paul.
PS On 28 Jul 2015, at 10:37, Tagir F. Valeev amae...@gmail.com wrote:
Hello!
Current implementation of Collections.nCopies().stream() is as
follows:
http://hg.openjdk.java.net/jdk9/dev/jdk/file/f160dec9a350/src/java.base/share/classes/java/util/Collections.java#l5066
public StreamE stream() {
return IntStream.range(0, n).mapToObj(i - element);
}
@Override
public StreamE parallelStream() {
return IntStream.range(0, n).parallel().mapToObj(i - element);
}
The problem is that it adds a lambda expression to the
RangeIntSpliterator type profile which can be polluted by some other
code and vice versa: using nCopies().stream() may pollute the type
profile for other code making it slower.
Another thing which is missing in current implementation is unordered
mode. This collection is unordered by nature, its stream is similar to
Stream.generate(), so to my opinion it should be unordered which may
improve the parallel reduction in some cases.
This can be improved by introducing the custom spliterator class which
is quite simple:
https://gist.github.com/amaembo/62f3efee9923b1468e86
On pre-polluted type profile with simple mapping and reduction using
custom spliterator is about 25-30% faster in both parallel and
sequential cases as benchmarking shows (performed on 4-core cpu).
What do you think?
With best regards,
Tagir Valeev.
Re: RFR(S): 8132704: [TESTBUG] jdk/internal/jimage/ExecutableTest.java incorrectly asserts all files to be executable
On Thu, Jul 30, 2015 at 5:47 PM, Roger Riggs roger.ri...@oracle.com wrote: Hi Volker, There is already as task for JMC to move that file but who knows exactly when. A list of exception(s) would be better I think, because generally, everything in bin should be executable. WDYT? OK, I'm fine with that. Unfortunately I'll have to hurry home now. I'll do the change tomorrow (or you can take over if it's urgent). Regards, Volker Roger On 7/30/2015 11:40 AM, Volker Simonis wrote: On Thu, Jul 30, 2015 at 5:34 PM, Roger Riggs roger.ri...@oracle.com wrote: Hi Volker, Possibly the real bug is that there is non-executable file in the bin directory. There is a /conf directory which would probably be a better place for that. Yes, I agree. But I thought you want a fast fix for the test failure :) Moving that config file is probably a bigger effort. Moreover the bin/ directory on Windows also contains .dll and .diz files. However on Windows, all the files seem to be executable (at least the test did succeed before). Nevertheless, checking only a known subset of executables seems safer and good enough. What do you think? Volker Roger On 7/30/2015 11:28 AM, Volker Simonis wrote: Hi, can somebody please review this test fix: http://cr.openjdk.java.net/~simonis/webrevs/2015/8132704/ https://bugs.openjdk.java.net/browse/JDK-8132704 The initial test checked that all the files in the bin/ directory are executable by everybody. Unfortunately this was too optimistic because in the closed build the bin/ directory contains configuration files which are not executable. The new version of the test uses a predefined static list of executables which are checked for the executable permissions if the corresponding files exist. Thank you and best regards, Volker
RFR 8130828: Fix some typos and omissions in the the j.u.stream JavaDoc
Hi Stefan, Tagir, Updated: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8130828-stream-doc-typos/webrev/ Paul. On 30 Jul 2015, at 18:08, Stefan Zobel splitera...@gmail.com wrote: Hi Paul, perhaps you could take the opportunity and also add the missing @since 1.9 tags to all the new dropWhile() / takeWhile() methods (JDK-8071597). Also in dropWhile() / takeWhile() there is a small typo (I guess) in the Javadoc: takeWhile: takes all elements (the result is the same is the input) dropWhile: the stream match the given predicate then no elements are dropped (the result is the same is the input) I guess that should read: (the result is the same as is the input)? Stefan
Re: RFR 9: JDK-8132705 : Refactor SharedSecrets in sun.misc.JavaNetAccess
On 30/07/2015 16:21, Roger Riggs wrote: Hi, I renamed the new interface and its uses as recommended. The webrev updated in place: http://cr.openjdk.java.net/~rriggs/webrev-inetaccess-8132705/ I deferred renaming JavaNetAccess because it (getURLClassPath) is used in install and deploy and I was not certain it would still be needed with the module system in place. It can be renamed (in a separate changeset) if that's useful. Thanks for the rename, looks good now. I don't think the getURLClassPath method will be relevant when we bring in the module system, mostly because the app class loader has not need to be a URLClassLoader. -Alan
Re: RFR Collector.finisher refers to IDENTITY_TRANSFORM rather than INDENTITY_FINISH
Hi Paul,
perhaps you could take the opportunity and also add the missing @since
1.9 tags to all the new dropWhile() / takeWhile() methods (JDK-8071597).
Also in dropWhile() / takeWhile() there is a small typo (I guess) in the
Javadoc:
takeWhile: takes all elements (the result is the same is the input)
dropWhile: the stream match the given predicate then no elements are
dropped (the result is the same is the input)
I guess that should read: (the result is the same as is the input)?
Stefan
2015-07-30 17:08 GMT+02:00 Paul Sandoz paul.san...@oracle.com:
Hi,
Please review this simple fix to the JavaDoc on
j.u.stream.Collector.finisher.
I am also opportunistically fixing some internal comments identified by
Tagir.
Paul.
diff -r 4e3135fac8cc
src/java.base/share/classes/java/util/stream/Collector.java
--- a/src/java.base/share/classes/java/util/stream/Collector.java
Fri Jul 24 15:33:13 2015 -0700
+++ b/src/java.base/share/classes/java/util/stream/Collector.java
Thu Jul 30 17:05:13 2015 +0200
@@ -223,7 +223,7 @@
* Perform the final transformation from the intermediate
accumulation type
* {@code A} to the final result type {@code R}.
*
- * pIf the characteristic {@code IDENTITY_TRANSFORM} is
+ * pIf the characteristic {@code IDENTITY_FINISH} is
* set, this function may be presumed to be an identity transform
with an
* unchecked cast from {@code A} to {@code R}.
*
diff -r 4e3135fac8cc
src/java.base/share/classes/java/util/stream/SliceOps.java
--- a/src/java.base/share/classes/java/util/stream/SliceOps.java
Fri Jul 24 15:33:13 2015 -0700
+++ b/src/java.base/share/classes/java/util/stream/SliceOps.java
Thu Jul 30 17:05:13 2015 +0200
@@ -138,7 +138,7 @@
skip, limit, size);
}
else {
-// @@@ OOMEs will occur for
LongStream.longs().filter(i - true).limit(n)
+// @@@ OOMEs will occur for LongStream.range(0,
Long.MAX_VALUE)).filter(i - true).limit(n)
// regardless of the value of n
// Need to adjust the target size of splitting
for the
// SliceTask from say (size / k) to say min(size
/ k, 1 14)
diff -r 4e3135fac8cc
src/java.base/share/classes/java/util/stream/Streams.java
--- a/src/java.base/share/classes/java/util/stream/Streams.java Fri Jul 24
15:33:13 2015 -0700
+++ b/src/java.base/share/classes/java/util/stream/Streams.java Thu Jul 30
17:05:13 2015 +0200
@@ -156,10 +156,9 @@
* than a balanced tree at the expense of a higher-depth for the
right
* side of the range.
*
- * pThis is optimized for cases such as IntStream.ints() that is
- * implemented as range of 0 to Integer.MAX_VALUE but is likely
to be
- * augmented with a limit operation that limits the number of
elements
- * to a count lower than this threshold.
+ * pThis is optimized for cases such as IntStream.range(0,
Integer.MAX_VALUE)
+ * that is likely to be augmented with a limit operation that
limits the
+ * number of elements to a count lower than this threshold.
*/
private static final int BALANCED_SPLIT_THRESHOLD = 1 24;
@@ -280,10 +279,9 @@
* than a balanced tree at the expense of a higher-depth for the
right
* side of the range.
*
- * pThis is optimized for cases such as LongStream.longs() that
is
- * implemented as range of 0 to Long.MAX_VALUE but is likely to be
- * augmented with a limit operation that limits the number of
elements
- * to a count lower than this threshold.
+ * pThis is optimized for cases such as LongStream.range(0,
Long.MAX_VALUE)
+ * that is likely to be augmented with a limit operation that
limits the
+ * number of elements to a count lower than this threshold.
*/
private static final long BALANCED_SPLIT_THRESHOLD = 1 24;
Re: RfR - 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java
On 30/07/15 17:55, huizhe wang wrote: Hi Daniel, On 7/30/2015 6:38 AM, Daniel Fuchs wrote: Hi, Please find below an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.01/ Looks good to me. The only nit is it seems createDefaultErrorHandler method is a dup of createDefaultErrorListener? Yes it is - but the return type is different. Instead of removing the CLITest.java - I copied Process.java into the test hierarchy, renamed it to ProcessXSLT.java, modified it so that it no longer uses internal APIs, and changed the CLITest.java to use that. This is a good idea as it keeps the Process utility around. The only adherence is with com.sun.org.apache.xml.internal.utils.DefaultErrorHandler that the new ProcessXSLT class tries to instantiate through reflection - as that error handler has access to jaxp internals and can give a better error diagnostic. However - the ProcessXSLT will use its own dummy ErrorHandler if it can't managed to instantiate the internal class. This way - we can keep the test :-) Perfect indeed, and if others (Yuri) want to use it, it's readily usable. Thanks Joe! -- daniel best, Joe best regards, -- daniel On 29/07/15 17:02, Daniel Fuchs wrote: Hi, Please find below a patch that removes a bunch of unused files in jdk9/dev/jaxp: https://bugs.openjdk.java.net/browse/JDK-8130058 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.00/ best regards, -- daniel
Re: RfR - 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java
On 7/30/2015 9:08 AM, Daniel Fuchs wrote: On 30/07/15 17:55, huizhe wang wrote: Hi Daniel, On 7/30/2015 6:38 AM, Daniel Fuchs wrote: Hi, Please find below an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.01/ Looks good to me. The only nit is it seems createDefaultErrorHandler method is a dup of createDefaultErrorListener? Yes it is - but the return type is different. Yes, but didn't see it's used. Joe Instead of removing the CLITest.java - I copied Process.java into the test hierarchy, renamed it to ProcessXSLT.java, modified it so that it no longer uses internal APIs, and changed the CLITest.java to use that. This is a good idea as it keeps the Process utility around. The only adherence is with com.sun.org.apache.xml.internal.utils.DefaultErrorHandler that the new ProcessXSLT class tries to instantiate through reflection - as that error handler has access to jaxp internals and can give a better error diagnostic. However - the ProcessXSLT will use its own dummy ErrorHandler if it can't managed to instantiate the internal class. This way - we can keep the test :-) Perfect indeed, and if others (Yuri) want to use it, it's readily usable. Thanks Joe! -- daniel best, Joe best regards, -- daniel On 29/07/15 17:02, Daniel Fuchs wrote: Hi, Please find below a patch that removes a bunch of unused files in jdk9/dev/jaxp: https://bugs.openjdk.java.net/browse/JDK-8130058 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.00/ best regards, -- daniel
RFR(xs): 8132745: minor cleanup of java/util/Scanner/ScanTest.java
Hi all, Please review this quick cleanup to this test I moved into the open yesterday. It changes the JVM's locale, and out of an abundance of caution it's safer to run things that change JVM global state in /othervm mode. Bug: https://bugs.openjdk.java.net/browse/JDK-8132745 Patch appended below. Thanks, s'marks # HG changeset patch # User smarks # Date 1438295744 25200 # Thu Jul 30 15:35:44 2015 -0700 # Node ID 4ce7979b7610574075bd4ade3b73004cde5e9ac3 # Parent d23203801b5369603d2dda21a6aff6225195001d 8132745: minor cleanup of java/util/Scanner/ScanTest.java Reviewed-by: XXX diff -r d23203801b53 -r 4ce7979b7610 test/java/util/Scanner/ScanTest.java --- a/test/java/util/Scanner/ScanTest.java Thu Jul 30 14:16:58 2015 -0400 +++ b/test/java/util/Scanner/ScanTest.java Thu Jul 30 15:35:44 2015 -0700 @@ -26,6 +26,7 @@ * @bug 4313885 4926319 4927634 5032610 5032622 5049968 5059533 6223711 6277261 6269946 6288823 * @summary Basic tests of java.util.Scanner methods * @key randomness + * @run main/othervm ScanTest */ import java.util.*;
RFR JDK-8080252: java.util.Formatter documentation of %n converter is misleading
Hi, Please help review the change for issue: https://bugs.openjdk.java.net/browse/JDK-8080252 webrev: http://cr.openjdk.java.net/~sherman/8080252/ It appears we updated the j.u.Formatter implementation to use the newly introduced method System.lineSeparator() in jdk7, but did not update the doc accordingly. For the old the behavior, I would assume the initial intent is to treat the system property line.separator as a read-only/informative property. Given the fact the it has been 2 major releases (7, 8), I would assume this change is now a kind of change to update the javadoc to describe the existing implementation correctly. So a CCC is not needed. Thanks, Sherman
Re: RFR 8130828: Fix some typos and omissions in the the j.u.stream JavaDoc
Hi Paul, looks good. Stefan 2015-07-30 18:32 GMT+02:00 Paul Sandoz paul.san...@oracle.com: Hi Stefan, Tagir, Updated: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8130828-stream-doc-typos/webrev/ Paul.
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 07/30/2015 12:24 PM, David Holmes wrote:
On 30/07/2015 8:20 PM, Peter Levart wrote:
On 07/30/2015 11:12 AM, Daniel Fuchs wrote:
Hi Peter,
I'm glad you're looking at this too! We can't have too many eyes :-)
On 30/07/15 10:38, Peter Levart wrote:
Suppose we have a Reference 'r' and it's associated ReferenceQueue
'q'.
Currently it can happen that the following evaluates to true, which is
surprising:
q.poll() == null r.isEnqueued()
But on the other hand this can only happen if two different
threads are polling the queue - in which case only one of them
will get the reference. In such a situation, the symmetric condition
would not be surprising (as the other thread would
get q.poll() != null):
r.isEnqueued() q.poll() == null
The original bug fixed by Kim is more surprising, because there's only
one Thread doing the polling, and it does get:
r.isEnqueud() q.poll() == null
although nobody else is polling the queue.
This should no longer occur in this situation with Kim's fix.
cheers,
-- daniel
Yes, these are two different issues. The one Kim has fixed is the race
of above expressions with Queue.enqueue() (when the queue is changing
from the associated instance to ENQUEUED). The one I'm pointing at and
Kim has already identified as potential issue is the race of the
following expression:
r.isEnqueued() q.poll() == null r.isEnqueued() == true
with Queue.[realy]poll() (when the queue is changing from ENQUEUED
to NULL).
Which only happens if at least two threads are polling the queue, but is
equally surprising and prone to cause bugs, don't you think?
Sorry I'm missing your point. The expression:
r.isEnqueued() q.poll() == null
is exactly the race the current fix is addressing. Adding a second
check of r.isEnqueued() which still returns true does not add anything
that I can see. ??
David
The expressions are similar, but the race is different. The one
addressed by Kim is the race of:
r.isEnqueued() q.poll() == null == true
with q.enqueue(r)
There, the reversal of assignments to q.head and r.queue in
ReferenceQueue.enqueue() fixes the issue.
The one I'm pointing at is the race of:
r.isEnqueued() q.poll() == null r.isEnqueued() == true
with q.poll()
Here, the fix would be to also revert the assignments to q.head and
r.queue in ReferenceQueue.reallyPoll() this time.
The 1st race is the race with enqueue-ing and the 2nd race is the race
with de-queueing. Initially, my surprising expression was:
q.poll() == null r.isEnqueued() == true
...but this is not representative, as it is also an expected outcome
when racing with enqueue-ing. So I added a pre-condition to express the
fact that it happens when racing with de-queueing only:
r.isEnqueued() q.poll() == null r.isEnqueued() == true
What is surprising in the above expression evaluating to true is the
fact that 'r' appears to be enqueued before and after the q.poll()
returns null. I can easily imagine code that would fail because it never
imagined above expression to evaluate to true. For example:
if (r.isEnqueued()) {
Reference s = q.poll();
if (s == null) {
// somebody else already dequeued 'r'
assert !r.isEnqueued();
}
}
Regards, Peter
Regards, Peter
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 07/30/2015 11:12 AM, Daniel Fuchs wrote: Hi Peter, I'm glad you're looking at this too! We can't have too many eyes :-) On 30/07/15 10:38, Peter Levart wrote: Suppose we have a Reference 'r' and it's associated ReferenceQueue 'q'. Currently it can happen that the following evaluates to true, which is surprising: q.poll() == null r.isEnqueued() But on the other hand this can only happen if two different threads are polling the queue - in which case only one of them will get the reference. In such a situation, the symmetric condition would not be surprising (as the other thread would get q.poll() != null): r.isEnqueued() q.poll() == null The original bug fixed by Kim is more surprising, because there's only one Thread doing the polling, and it does get: r.isEnqueud() q.poll() == null although nobody else is polling the queue. This should no longer occur in this situation with Kim's fix. cheers, -- daniel Yes, these are two different issues. The one Kim has fixed is the race of above expressions with Queue.enqueue() (when the queue is changing from the associated instance to ENQUEUED). The one I'm pointing at and Kim has already identified as potential issue is the race of the following expression: r.isEnqueued() q.poll() == null r.isEnqueued() == true with Queue.[realy]poll() (when the queue is changing from ENQUEUED to NULL). Which only happens if at least two threads are polling the queue, but is equally surprising and prone to cause bugs, don't you think? Regards, Peter
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
On 30/07/2015 8:20 PM, Peter Levart wrote: On 07/30/2015 11:12 AM, Daniel Fuchs wrote: Hi Peter, I'm glad you're looking at this too! We can't have too many eyes :-) On 30/07/15 10:38, Peter Levart wrote: Suppose we have a Reference 'r' and it's associated ReferenceQueue 'q'. Currently it can happen that the following evaluates to true, which is surprising: q.poll() == null r.isEnqueued() But on the other hand this can only happen if two different threads are polling the queue - in which case only one of them will get the reference. In such a situation, the symmetric condition would not be surprising (as the other thread would get q.poll() != null): r.isEnqueued() q.poll() == null The original bug fixed by Kim is more surprising, because there's only one Thread doing the polling, and it does get: r.isEnqueud() q.poll() == null although nobody else is polling the queue. This should no longer occur in this situation with Kim's fix. cheers, -- daniel Yes, these are two different issues. The one Kim has fixed is the race of above expressions with Queue.enqueue() (when the queue is changing from the associated instance to ENQUEUED). The one I'm pointing at and Kim has already identified as potential issue is the race of the following expression: r.isEnqueued() q.poll() == null r.isEnqueued() == true with Queue.[realy]poll() (when the queue is changing from ENQUEUED to NULL). Which only happens if at least two threads are polling the queue, but is equally surprising and prone to cause bugs, don't you think? Sorry I'm missing your point. The expression: r.isEnqueued() q.poll() == null is exactly the race the current fix is addressing. Adding a second check of r.isEnqueued() which still returns true does not add anything that I can see. ?? David Regards, Peter
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with RuntimeException: Error: poll() returned null; expected ref object
Hi Peter, I'm glad you're looking at this too! We can't have too many eyes :-) On 30/07/15 10:38, Peter Levart wrote: Suppose we have a Reference 'r' and it's associated ReferenceQueue 'q'. Currently it can happen that the following evaluates to true, which is surprising: q.poll() == null r.isEnqueued() But on the other hand this can only happen if two different threads are polling the queue - in which case only one of them will get the reference. In such a situation, the symmetric condition would not be surprising (as the other thread would get q.poll() != null): r.isEnqueued() q.poll() == null The original bug fixed by Kim is more surprising, because there's only one Thread doing the polling, and it does get: r.isEnqueud() q.poll() == null although nobody else is polling the queue. This should no longer occur in this situation with Kim's fix. cheers, -- daniel
