hg: jdk8/tl/langtools: 8024415: Bug in javac Pretty: Wrong precedence in JCConditional trees
Changeset: ea000904db62 Author:alundblad Date: 2013-10-08 15:33 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/ea000904db62 8024415: Bug in javac Pretty: Wrong precedence in JCConditional trees Summary: Fixed precedence and associativity issues with pretty printing of JCConditional expressions. Reviewed-by: jfranck Contributed-by: Andreas Lundblad andreas.lundb...@oracle.com, Matthew Dempsky mdemp...@google.com ! src/share/classes/com/sun/tools/javac/tree/Pretty.java + test/tools/javac/tree/T8024415.java
Re: JDK 8 code review request forJDK-8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation
On 10/08/2013 08:08 PM, Mike Duigou wrote:
This seems to contradict the main documentation for these methods.
The general guidance is still valid, bug we want to allow algorithms
with better worst-case behavior than simple summation.
(I didn't think it would be helpful for most readers to start talking
about error bounds or to otherwise explain the desired quality of
implementation constraints on the behavior of the method.)
Perhaps instead we should remove the The average returned can vary depending upon
the order in which values are recorded. This is due to accumulated rounding error in
addition of values of differing magnitudes. Values sorted by increasing absolute
magnitude tend to yield more accurate results. into an @implNote?
I also suspect that having this documentation only in DoubleSummaryStatistics
may be too hidden away. Perhaps similar docs on DoubleStream sum() and
average() methods as well?
Numerically, using similar techniques in DoubleStream is preferable, but
specification like
/**
* Returns the sum of elements in this stream. The sum returned
can vary
* depending upon the order in which elements are encountered. This
is due
* to accumulated rounding error in addition of values of differing
* magnitudes. Elements sorted by increasing absolute magnitude tend to
* yield more accurate results. If any stream element is a {@code
NaN} or
* the sum is at any point a {@code NaN} then the sum will be
{@code NaN}.
* This is a special case of a
* a href=package-summary.html#Reductionreduction/a and is
* equivalent to:
* pre{@code
* return reduce(0, Double::sum);
* }/pre
*
* pThis is a a href=package-summary.html#StreamOpsterminal
* operation/a.
*
* @return the sum of elements in this stream
*/
double sum();
would need to be updated.
-Joe
Mike
On Oct 8 2013, at 18:56 , Joe Darcy wrote:
Hello,
Please review the patch below which addresses
JDK-8024354 Explicitly permit DoubleStream.sum()/average()
implementations to use higher precision summation
Thanks,
-Joe
diff -r f1e31376f419 src/share/classes/java/util/DoubleSummaryStatistics.java
--- a/src/share/classes/java/util/DoubleSummaryStatistics.java Wed Oct 09
00:10:02 2013 +0100
+++ b/src/share/classes/java/util/DoubleSummaryStatistics.java Tue Oct 08
18:54:55 2013 -0700
@@ -118,6 +118,11 @@
* value is a {@code NaN} or the sum is at any point a {@code NaN} then the
* sum will be {@code NaN}.
*
+ * @implNote This method may be implemented using compensated
+ * summation or other technique to reduce the error bound in the
+ * numerical sum compared to a simple summation of {@code double}
+ * values.
+ *
* @return the sum of values, or zero if none
*/
public final double getSum() {
@@ -161,6 +166,10 @@
* value is a {@code NaN} or the sum is at any point a {@code NaN} then the
* average will be {@code NaN}.
*
+ * @implNote This method may be implemented using compensated
+ * summation or other technique to reduce the error bound in the
+ * numerical sum used to compute the average.
+ *
* @return the arithmetic mean of values, or zero if none
*/
public final double getAverage() {
Re: Review request for 8026027: Level.parse should return the custom Level instance instead of the mirrored Level
On 10/9/13 2:39 AM, Mandy Chung wrote: This fixes Level.parse to return the custom Level instance. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8026027/webrev.00/ When a custom Level is created, a mirrored level instance (containing the same value as the custom Level) is created and used by the logging implementation. Only the custom level should be added to the known level list from which Level.parse will look up. Thanks Mandy Hi Mandy, This looks good - but I think you could move the changes line 554-562 and put them back inside the KnownLevel constructor where they were before. This would allow you to keep mirroredLevel final. Small nit: the name 'custom' is a misnomer - as it will be true for both standard and custom levels... Concerning the test I don't think you need to copy the property file to test.classes, because I believe jprt puts test.src inside the classpath. (another possibility would be to use a custom subclass of ListResourceBundle instead) Finally, I think that test needs to be run in main/othervm mode - otherwise it might fail intermittently... best regards, -- daniel
Re: [7u-dev] Request for review of backport 7147084
Alan, Rob, Martin, Would you please help review the backport? I see that you were the reviewers for the master fix. I need a review for the backport, since the patch couldn't get applied cleanly. Here are the changes I had to make: - I had to manually replace Java_java_lang_ProcessImpl_create() function body with the new version, as 'hg patch' could not handle it automatically. - I had to replace wide-char strings to regular char*, in all the calls to win32Error() function. Thanks in advance, Ivan On 25.09.2013 17:50, Ivan Gerasimov wrote: Hello! May I please have a review for a backport of the fix for 7147084 to jdk7u-dev? This webrev does not include fix 8007454 anymore, as it has already been pushed. http://cr.openjdk.java.net/~igerasim/7147084/1/webrev/ Thanks in advance, Ivan Gerasimov Original Message Subject: Re: [7u-dev] Request for approval for CRs 8007454, 7147084 Date: Tue, 24 Sep 2013 10:29:04 +0100 From: Seán Coffey sean.cof...@oracle.com To: Ivan Gerasimov ivan.gerasi...@oracle.com CC: jdk7u-...@openjdk.java.net On 24/09/2013 10:25, Ivan Gerasimov wrote: Thanks Seán! Then I'll ask for an approval to backport 8007454, which is applied cleanly. Consider both bugs fixes approved. You could use this mail thread to review the 7147084 changes if you want. They're innocent enough. Alan or someone else should be able to review. regards, Sean. And once it is pushed, I'll initiate a review for 7147084 with adjustments. Sincerely, Ivan On 24.09.2013 12:59, Seán Coffey wrote: Approved for jdk7u-dev but you'll need a review first due to the adjustments needed in the backport. Hopefully, you'll be resolving these issues with 2 different changesets. It's been the trend in the past and helps keep changesets more aligned to those in JDK 8. regards, Sean. On 24/09/2013 09:08, Ivan Gerasimov wrote: Sending to the correct ML -- Hello! We have a request to backport fix for 7147084. First, it depends on the fix for 8007454, so the webrev includes it too. Second, the fix had to be adjusted a bit. - I had to manually replaced Java_java_lang_ProcessImpl_create() function body with the new version, as 'hg patch' could not handle automatically. - I had to replace wide-char strings to regular char*, in all the calls to win32Error() function. Combined webrev: http://cr.openjdk.java.net/~igerasim/7147084/0/webrev/ BUGS: http://bugs.sun.com/view_bug.do?bug_id=7147084 http://bugs.sun.com/view_bug.do?bug_id=8007454 JDK8 Changesets: 7147084: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2c4f1081a0fa 8007454: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0a2b308cc82d Reviews for jdk8 fix: 7147084: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-August/019604.html 8007454: http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-February/014602.html I had set up a JPRT job, which completed successfully. All the .*jdk_lang.* test passed. Sincerely yours, Ivan Gerasimov
Re: Time to put a stop to Thread.stop?
Alan Bateman wrote: On 25/05/2013 12:35, Alan Bateman wrote: On 24/05/2013 19:14, Martin Buchholz wrote: : Alan, you're telling everyone there's no need for Thread.stop, but you are replacing usages in tests with calls to Unsafe, which is not available to normal code. So you have a kind of moral obligation here to replace usages with ordinary java code. There are other ways to do sneakyThrow, and perhaps a sneakyRethrow method should be added to the jdk test library. For these the tests then it shouldn't really matter but if you or Doug would prefer if they are changed to use sneakyThrow then we can do that. Martin has pushed updates to the 4 tests to the jsr166 CVS so I'll going to push these to jdk8/tl forest along with the Thread.stop changes. I noticed the other day when using Java 8 that Thread.stop(Throwable) was throwing an UOE. Unfortunately I missed the discussion in May amongst the thousands of other important JDK emails. Thus I presume that nothing I might have to say on the matter would change what is currently in the OpenJDK ;-) But I'd like to state it for the record anyway. Throwing checked exceptions in an unchecked context is, IMHO, something one should almost never do, except maybe in test cases. Up to now, I've known of quite a few ways to do this though: 1. Thread.stop(Throwable) with some mangling - see ThrowerConcurrent in http://www.javaspecialists.eu/archive/Issue144.html 2. Abusing generics 3. Class.newInstance(), described in Java Puzzlers (http://www.javaspecialists.eu/archive/Issue144.html) 4. Unsafe.throwException() similar to ThrowerConcurrent in #1. Since this is something that we should not do anyway, it is no harm to remove one of the ways, this being Thread.stop(Throwable). However, there is another use case. If you cause a deadlock with ReentrantLock, you might be able to bust out of that by sending a DeadlockVictimError to one of the threads in the deadlock. This would not work for threads deadlocked with synchronized. I agree this is a rare situation and we should rather avoid the deadlock. But it is useful to be able to do it. Here are two newsletters where I write about this: http://www.javaspecialists.eu/archive/Issue130.html http://www.javaspecialists.eu/archive/Issue160.html Now it is enough of an edge case that I can accept using Unsafe.throwException() as being the proper solution. Since it is unsafe, we might as well use that mechanism. However, it is yet another temptation to use Unsafe, which, of course, we should avoid, seeing that it is, ahem, unsafe ;-) Regards Heinz
Re: JDK 8 RFC 7189139 version 2: BigInteger's staticRandom field can be a source of bottlenecks
On 10/09/2013 12:05 AM, Brian Burkhalter wrote: Based on previous comments by Aleksey and Paul I created this test http://cr.openjdk.java.net/~bpb/7189139/PrimeTest.java and ran it with the list of primes supplied by Aleksey, N=6000, and certainty=100. This is the output: --- Primes: /Users/bpb/Desktop/primes-50M.txt N = 6000 certainty = 100 3562115 probable primes out of 3562115 candidates Prime test = true Not-prime test = true Test succeeded! Awesome, thanks! An updated webrev which differs only in having a correct Hg header is here: http://cr.openjdk.java.net/~bpb/7189139/webrev.2/ Thumbs up for the fix. -Aleksey.
RFR [8023390] Test java/net/NetworkInterface/MemLeakTest.java failed
Hello all! The MemLeakTest had been added with the fix for 8022584. Since that, the test was reported to fail intermittently, even though the leak was eliminated. I couldn't ever reproduce the failure even on the machines where the failure was detected. Here are the changes I propose: - Increase number of both warm-up and measured iterations, - Number of iterations now indicates how many times a single interface is probed. It used to probe all the interfaces that many times. - Increase the memory consumption threshold - Increase the timeout These should add some confidence that the failure of the test really indicates a memory leak. In addition to that, in the case of a failure the list of all the network interfaces is displayed, so there will be some more information about the environment. Here is the webrev: http://cr.openjdk.java.net/~igerasim/8023390/0/webrev/ Sincerely yours, Ivan Gerasimov
Re: RFR: 8024704: Improve API documentation of ClassLoader and ServiceLoader with respect to enumeration of resources.
On 10/8/2013 1:06 PM, Alan Bateman wrote: On 08/10/2013 18:19, Daniel Fuchs wrote: Hi, Please find below a fix for: 8024704: Improve API documentation of ClassLoader and ServiceLoader with respect to enumeration of resources. https://bugs.openjdk.java.net/browse/JDK-8024704 This is a clarification of the implementation of the ServiceLoader.iterator() method, as well as non normative advice for ClassLoader subclasses overriding getResource() or getResources() to consider overriding the other method in order to keep them consistent with each other. http://cr.openjdk.java.net/~dfuchs/webrev_8024704/webrev.00/ Daniel - the spec update looks okay to me. As background to others, the motive for this one stems from a small compatibility issue that arose with the JAXP changes to use ServiceLoader (it was previous foraging for service configuration files itself). The compatibility issue arises with ClassLoader implementations where getResource and getResources are inconsistent, and in the JAXP case uncovered a server that located an unexpected XML parser. It'd be interesting to find out how many custom ClassLoader implements the getResource and getResources inconsistently but this might not be easy. Mandy
Re: RFR [8023390] Test java/net/NetworkInterface/MemLeakTest.java failed
This looks like one for the net-dev list. On 09/10/2013 09:10, Ivan Gerasimov wrote: Hello all! The MemLeakTest had been added with the fix for 8022584. Since that, the test was reported to fail intermittently, even though the leak was eliminated. I couldn't ever reproduce the failure even on the machines where the failure was detected. Here are the changes I propose: - Increase number of both warm-up and measured iterations, - Number of iterations now indicates how many times a single interface is probed. It used to probe all the interfaces that many times. - Increase the memory consumption threshold - Increase the timeout These should add some confidence that the failure of the test really indicates a memory leak. In addition to that, in the case of a failure the list of all the network interfaces is displayed, so there will be some more information about the environment. Here is the webrev: http://cr.openjdk.java.net/~igerasim/8023390/0/webrev/ Sincerely yours, Ivan Gerasimov
hg: jdk8/tl/jdk: 8008662: Add @jdk.Exported to JDK-specific/exported APIs
Changeset: 2ea162b2ff55 Author:alanb Date: 2013-10-09 09:20 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2ea162b2ff55 8008662: Add @jdk.Exported to JDK-specific/exported APIs Reviewed-by: chegar, vinnie, dfuchs, mchung, mullan, darcy ! src/share/classes/com/sun/jdi/AbsentInformationException.java ! src/share/classes/com/sun/jdi/Accessible.java ! src/share/classes/com/sun/jdi/ArrayReference.java ! src/share/classes/com/sun/jdi/ArrayType.java ! src/share/classes/com/sun/jdi/BooleanType.java ! src/share/classes/com/sun/jdi/BooleanValue.java ! src/share/classes/com/sun/jdi/Bootstrap.java ! src/share/classes/com/sun/jdi/ByteType.java ! src/share/classes/com/sun/jdi/ByteValue.java ! src/share/classes/com/sun/jdi/CharType.java ! src/share/classes/com/sun/jdi/CharValue.java ! src/share/classes/com/sun/jdi/ClassLoaderReference.java ! src/share/classes/com/sun/jdi/ClassNotLoadedException.java ! src/share/classes/com/sun/jdi/ClassNotPreparedException.java ! src/share/classes/com/sun/jdi/ClassObjectReference.java ! src/share/classes/com/sun/jdi/ClassType.java ! src/share/classes/com/sun/jdi/DoubleType.java ! src/share/classes/com/sun/jdi/DoubleValue.java ! src/share/classes/com/sun/jdi/Field.java ! src/share/classes/com/sun/jdi/FloatType.java ! src/share/classes/com/sun/jdi/FloatValue.java ! src/share/classes/com/sun/jdi/IncompatibleThreadStateException.java ! src/share/classes/com/sun/jdi/InconsistentDebugInfoException.java ! src/share/classes/com/sun/jdi/IntegerType.java ! src/share/classes/com/sun/jdi/IntegerValue.java ! src/share/classes/com/sun/jdi/InterfaceType.java ! src/share/classes/com/sun/jdi/InternalException.java ! src/share/classes/com/sun/jdi/InvalidCodeIndexException.java ! src/share/classes/com/sun/jdi/InvalidLineNumberException.java ! src/share/classes/com/sun/jdi/InvalidStackFrameException.java ! src/share/classes/com/sun/jdi/InvalidTypeException.java ! src/share/classes/com/sun/jdi/InvocationException.java ! src/share/classes/com/sun/jdi/JDIPermission.java ! src/share/classes/com/sun/jdi/LocalVariable.java ! src/share/classes/com/sun/jdi/Locatable.java ! src/share/classes/com/sun/jdi/Location.java ! src/share/classes/com/sun/jdi/LongType.java ! src/share/classes/com/sun/jdi/LongValue.java ! src/share/classes/com/sun/jdi/Method.java ! src/share/classes/com/sun/jdi/Mirror.java ! src/share/classes/com/sun/jdi/MonitorInfo.java ! src/share/classes/com/sun/jdi/NativeMethodException.java ! src/share/classes/com/sun/jdi/ObjectCollectedException.java ! src/share/classes/com/sun/jdi/ObjectReference.java ! src/share/classes/com/sun/jdi/PathSearchingVirtualMachine.java ! src/share/classes/com/sun/jdi/PrimitiveType.java ! src/share/classes/com/sun/jdi/PrimitiveValue.java ! src/share/classes/com/sun/jdi/ReferenceType.java ! src/share/classes/com/sun/jdi/ShortType.java ! src/share/classes/com/sun/jdi/ShortValue.java ! src/share/classes/com/sun/jdi/StackFrame.java ! src/share/classes/com/sun/jdi/StringReference.java ! src/share/classes/com/sun/jdi/ThreadGroupReference.java ! src/share/classes/com/sun/jdi/ThreadReference.java ! src/share/classes/com/sun/jdi/Type.java ! src/share/classes/com/sun/jdi/TypeComponent.java ! src/share/classes/com/sun/jdi/VMCannotBeModifiedException.java ! src/share/classes/com/sun/jdi/VMDisconnectedException.java ! src/share/classes/com/sun/jdi/VMMismatchException.java ! src/share/classes/com/sun/jdi/VMOutOfMemoryException.java ! src/share/classes/com/sun/jdi/Value.java ! src/share/classes/com/sun/jdi/VirtualMachine.java ! src/share/classes/com/sun/jdi/VirtualMachineManager.java ! src/share/classes/com/sun/jdi/VoidType.java ! src/share/classes/com/sun/jdi/VoidValue.java ! src/share/classes/com/sun/jdi/connect/AttachingConnector.java ! src/share/classes/com/sun/jdi/connect/Connector.java ! src/share/classes/com/sun/jdi/connect/IllegalConnectorArgumentsException.java ! src/share/classes/com/sun/jdi/connect/LaunchingConnector.java ! src/share/classes/com/sun/jdi/connect/ListeningConnector.java ! src/share/classes/com/sun/jdi/connect/Transport.java ! src/share/classes/com/sun/jdi/connect/TransportTimeoutException.java ! src/share/classes/com/sun/jdi/connect/VMStartException.java + src/share/classes/com/sun/jdi/connect/package-info.java - src/share/classes/com/sun/jdi/connect/package.html ! src/share/classes/com/sun/jdi/connect/spi/ClosedConnectionException.java ! src/share/classes/com/sun/jdi/connect/spi/Connection.java ! src/share/classes/com/sun/jdi/connect/spi/TransportService.java + src/share/classes/com/sun/jdi/connect/spi/package-info.java - src/share/classes/com/sun/jdi/connect/spi/package.html ! src/share/classes/com/sun/jdi/event/AccessWatchpointEvent.java ! src/share/classes/com/sun/jdi/event/BreakpointEvent.java ! src/share/classes/com/sun/jdi/event/ClassPrepareEvent.java ! src/share/classes/com/sun/jdi/event/ClassUnloadEvent.java ! src/share/classes/com/sun/jdi/event/Event.java ! src/share/classes/com/sun/jdi/event/EventIterator.java !
Re: RFR [8023390] Test java/net/NetworkInterface/MemLeakTest.java failed
Yes, right, thanks! Just resend it to the right list On 09.10.2013 12:22, Alan Bateman wrote: This looks like one for the net-dev list. On 09/10/2013 09:10, Ivan Gerasimov wrote: Hello all! The MemLeakTest had been added with the fix for 8022584. Since that, the test was reported to fail intermittently, even though the leak was eliminated. I couldn't ever reproduce the failure even on the machines where the failure was detected. Here are the changes I propose: - Increase number of both warm-up and measured iterations, - Number of iterations now indicates how many times a single interface is probed. It used to probe all the interfaces that many times. - Increase the memory consumption threshold - Increase the timeout These should add some confidence that the failure of the test really indicates a memory leak. In addition to that, in the case of a failure the list of all the network interfaces is displayed, so there will be some more information about the environment. Here is the webrev: http://cr.openjdk.java.net/~igerasim/8023390/0/webrev/ Sincerely yours, Ivan Gerasimov
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
This latest webrev looks good to me. If you haven't already got a
sponsor, then I would be happy to sponsor this for you.
Since there are minor spec clarifications, to document long standing
behavior, then a CCC request will need to be submitted, and approved,
before integration.
-Chris.
On 08/10/2013 22:29, Brian Burkhalter wrote:
I have posted an updated webrev
http://cr.openjdk.java.net/~bpb/7179567/webrev.3/ which I hope addresses all
the concerns expressed below.
Please see also my comments in line.
Once this looks acceptable I can do the CCC request.
Thanks,
Brian
On Oct 7, 2013, at 9:28 PM, David Holmes wrote:
Aside: I'm confused about the relationship of this bug and JDK-6445180 - are
they not duplicates? Seems to me this one should have been closed as a dup when
it was reported.
I think you are correct. I can adjust this once the webrev is approved.
On 5/10/2013 6:58 AM, Brian Burkhalter wrote:
On Oct 3, 2013, at 5:38 PM, Brian Burkhalter wrote:
On Oct 3, 2013, at 5:35 PM, Alan Bateman wrote:
On 03/10/2013 16:10, Brian Burkhalter wrote:
Please review and comment at your convenience.
Issue: https://bugs.openjdk.java.net/browse/JDK-7179567
Webrev: http://cr.openjdk.java.net/~bpb/7179567/
An updated webrev which I hope adequately addresses the expressed concerns may
be found at:
http://cr.openjdk.java.net/~bpb/7179567.2/
URLClassLoader.java: please delete the commented out line:
+ //Objects.requireNonNull(urls);
I already fixed that but had not updated the webrev.
SecureClassLoader.java:
186 * @throws SecurityException if the {@code ClassLoader} instance is not
187 * initialized.
should read this classloader instance as it refers to the current instance. Also this
may be bringing the spec into line with the implementation but initialization here is
purely an implementation detail not part of the specification for this class so it should not be
mentioned explicitly - and this change still needs a CCC (which might help determine exactly what
should be said here - I'm unclear how you can try to call getPermissions if this is uninitialized
as it would need 'this' to escape from the constructor - which perhaps it can do via a third-party
security manager?).
I've removed this @throws clause.
NoCallStackClassLoader.java: the comment is far too verbose, we don't write
such explanatory kinds of comments for this kind of thing (else the code would
be overrun with commentary!).
Shortened.
Aside: if you are a JDK Author you don't need a Contributed-by line in the
commit comments.
Removed.
On Oct 8, 2013, at 4:08 AM, Alan Bateman wrote:
This looks much better.
If I read the code correctly then the long standing behavior of
URLClassLoader.getPermissions(null) was to throw a NPE, now it is changed to
return the empty collection in order to match the super type
(SecureClassLoader). I can't think of anything that would break so this is
probably okay, just maybe a bit surprising.
My previous comment on @throws vs. @exception was just to keep things
consistent as this is old code and would look odd for a method to use both. Our
preference is to switch to @throws whenever the opportunity arises. The comment
was also on the alignment/style. Take URLCLassLoader(URL[], ClassLoader) for
example, the existing SecurityException has its description aligned under
SecurityException whereas the new NullPointerException wraps around. So my
overall comment here was to avoid mixing styles (it doesn't matter too much
which style is used, just keep it consistent for future maintainers).
In the test then it might be better to change the @summary line to only include
the second paragraph (the bug summary is not interesting, especially when it
references a test that is not in OpenJDK).
Updated.
On Oct 8, 2013, at 8:53 AM, Chris Hegarty wrote:
Yes, this is much nicer.
I'd be tempted to write another private constructor, taking all args, and
checking params with checkForNullURLs before invoking, similar to ThreadGroup
[1] L117, for example. But that is just clean up to remove some duplication. No
need to do this.
If I read the code correctly then the long standing behavior of
URLClassLoader.getPermissions(null) was to throw a NPE, now it is
changed to return the empty collection in order to match the super type
(SecureClassLoader). I can't think of anything that would break so this
is probably okay, just maybe a bit surprising.
Agreed.
My previous comment on @throws vs. @exception was just to keep things
consistent as this is old code and would look odd for a method to use
both. Our preference is to switch to @throws whenever the opportunity
arises. The comment was also on the alignment/style. Take
URLCLassLoader(URL[], ClassLoader) for example, the existing
SecurityException has its description aligned under SecurityException
whereas the new NullPointerException wraps around. So my overall comment
here was to avoid mixing
Re: JDK 8 RFR 8016252: More defensive HashSet.readObject
On 08/10/2013 21:50, Brian Burkhalter wrote: http://cr.openjdk.java.net/~bpb/8016252/webrev.3/ This looks good to me. If you haven't already got a sponsor, then I would be happy to sponsor this for you. -Chris.
Re: RFR: 8024704: Improve API documentation of ClassLoader and ServiceLoader with respect to enumeration of resources.
Hi, Thank you all for the nice valuable feedback! I have updated the webrev with Alan's correction. The ClassLoader api note now reads: When overriding this method it is recommended that an implementation ensures that ... (and also fixed indentation) http://cr.openjdk.java.net/~dfuchs/webrev_8024704/webrev.01/ -- daniel On 10/8/13 10:06 PM, Alan Bateman wrote: On 08/10/2013 18:19, Daniel Fuchs wrote: Hi, Please find below a fix for: 8024704: Improve API documentation of ClassLoader and ServiceLoader with respect to enumeration of resources. https://bugs.openjdk.java.net/browse/JDK-8024704 This is a clarification of the implementation of the ServiceLoader.iterator() method, as well as non normative advice for ClassLoader subclasses overriding getResource() or getResources() to consider overriding the other method in order to keep them consistent with each other. http://cr.openjdk.java.net/~dfuchs/webrev_8024704/webrev.00/ As background to others, the motive for this one stems from a small compatibility issue that arose with the JAXP changes to use ServiceLoader (it was previous foraging for service configuration files itself). The compatibility issue arises with ClassLoader implementations where getResource and getResources are inconsistent, and in the JAXP case uncovered a server that located an unexpected XML parser. Daniel - in the @apiNote on getResource it reads the implementations ensure where it should be the implementation ensures or implementations should ensure. Otherwise the wording looks okay to me. Conventions haven't been established yet but I would think that @apiNote is a case where you can use the full line. -Alan.
Re: RFR: 8023524: Mechanism to dump generated lambda classes / log lambda code generation
Hi Henry, On 10/8/2013 10:57 PM, Henry Jen wrote: Hi, Please review updated webrev at http://cr.openjdk.java.net/~henryjen/ccc/8023524/6/webrev ProxyClassesDumper looks simpler after moving the path validation to the static factory method. One minor comment: ProxyClassesDumper.getInstance returns null if the given path is invalid. For a null path, it can simply return null rather than throwing NPE. It's good to see the limited doPrivileged being used here. I can see that listing the limited permissions would be a tradeoff with maintainability and readability. This also leads to the question how one can determine the complete list of permissions required (besides testing) and guideline on when to use limited privileged and when to use the entire ACC (Jeff may have some thoughts). Anyway, just some comments related to limited doPrivileged and not anything related to this fix. Henry - your fix looks good for me. You don't not need to generate a new webrev if you make any change per my comment about null path unless others have any other comment. thanks Mandy
Re: RFR 8024709 : TreeMap.DescendingKeyIterator 'remove' confuses iterator position
On 08/10/2013 20:55, Brent Christian wrote: I've beefed up the test case, as suggested by Alan and Paul. It tries removing at the start, middle, and end of the iteration. FWIW, all but one of the test scenarios pass even without the fix. The failing case is: checkDescItrRmMid(m.keySet(), m.navigableKeySet().descendingIterator()); (that is, remove an element from the middle of the iteration, the specific case of this bug). So most of the new tests are of code that is already correct, but IMO it doesn't hurt to make sure it stays correct. :) http://cr.openjdk.java.net/~bchristi/8024709/webrev.01/ Thanks for the update and expanding the test. I skimmed over the new test cases and they looks good. There are a few commented out cases and it's not clear why this also. Also a minor observation is that the method names are a bit inconsistent with the other method names in the test, they might be something to look at before pushing. -Alan
Re: Review request for 8026027: Level.parse should return the custom Level instance instead of the mirrored Level
Daniel, Thanks for the review. On 10/9/2013 12:38 AM, Daniel Fuchs wrote: This looks good - but I think you could move the changes line 554-562 and put them back inside the KnownLevel constructor where they were before. This would allow you to keep mirroredLevel final. That's right. I separated those lines in any earlier version and later added the private constructor per your suggestion. Small nit: the name 'custom' is a misnomer - as it will be true for both standard and custom levels... I renamed it to visible (visible levels can be found by Level.parse) Concerning the test I don't think you need to copy the property file to test.classes, because I believe jprt puts test.src inside the classpath. I think you meant jtreg. Thanks I have fixed that. (another possibility would be to use a custom subclass of ListResourceBundle instead) Finally, I think that test needs to be run in main/othervm mode - otherwise it might fail intermittently... Another good catch. Fixed. The updated webrev: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8026027/webrev.01/ Mandy
Re: JDK 8 RFR 8016252: More defensive HashSet.readObject
On 08/10/2013 21:50, Brian Burkhalter wrote: I have updated the webrev accordingly http://cr.openjdk.java.net/~bpb/8016252/webrev.3/ http://cr.openjdk.java.net/%7Ebpb/8016252/webrev.3/ This looks good, I think this gets us to where we wanted to be. : I skimmed over the test but it doesn't appear to exercise anything new. If you want to exercise the checks then it would require deserializing from a byte stream that results in bad loadFactor, size and capacity values. It might not be worth it of course. No, it does not exercise anything new. I actually did try inserting random garbage into the written-out byte stream, but without knowing where to do so to affect the fields of interest it is rather useless and causes totally unpredictable results. I don't know that this test really needs to be included with the patch. There are tools around for decoding serialization streams and in this case you need to change the right bytes to exercise the conditions. It's probably not worth spending too much time on this (there are more important things to get done before ZBB) so I wouldn't object to just creating another bug to improve the test coverage for this area and move on. -Alan.
Re: Deflater enhancements
See
https://bugs.openjdk.java.net/browse/JDK-8026127
https://bugs.openjdk.java.net/browse/JDK-8026128
Stephen
On 8 October 2013 21:33, Xueming Shen xueming.s...@oracle.com wrote:
The 100 in sample really means a big enough buffer here for simple use
case, not
necessarily means a fixed size' buffer. Sure there is always room for doc
improvement,
especially given this is the API has been there for decade.
Deflater/Inflater.end() is for explicitly/proactively release of the memory
resource
held by the deflater/inflater, it does not have impact to the
deflating/inflating result.
the end() will be invoked by the finalizer.
It might be reasonable to simply have a pair of static utility methods
byte[] Deflater.deflate(byte[]);
byte[] Inflater.deflate(byte[]);
For the casual/simple/easy use scenario.
Sherman
On 10/08/2013 03:24 AM, Stephen Colebourne wrote:
I've been trying to use Deflater today and found it rather tricky. Two
thoughts...
1) The class level Javadoc specifies a fixed size byte array of 100.
http://download.java.net/jdk8/docs/api/java/util/zip/Deflater.html
From what I can tell from other searching and basic common sense, this
cannot be right as the output size could well be bigger than 100.
However, there is no real information in the Deflater class as to how
the class is supposed to be used. I don't believe that kind of broken
example is helpful.
The best I found was
http://java.dzone.com/articles/how-compress-and-uncompress
which proposed a loop:
Deflater deflater = new Deflater();
deflater.setInput(data);
ByteArrayOutputStream outputStream = new
ByteArrayOutputStream(data.length);
deflater.finish();
byte[] buffer = new byte[1024];
while (!deflater.finished()) {
int count = deflater.deflate(buffer); // returns the generated code...
index
outputStream.write(buffer, 0, count);
}
outputStream.close();
byte[] output = outputStream.toByteArray();
Neither example call deflater.end(), which I believe to be a mistake as
well.
Is my analysis correct? Should I raise a bug for better documentation?
(this appears to affect Inflater as well)
2) Even with the Javadoc change, the API is still far too complex.
As a user, all I want is something like:
byte[] compressed = Deflater.deflate(input, options...)
and similar for Inflater.
(I actually want to compress a String, and an override to handle that
would be useful as well)
Any thoughts on adding a convenience method to make the API a lot
easier? Can I raise a bug for that?
Stephen
Re: Review request for 8026027: Level.parse should return the custom Level instance instead of the mirrored Level
Hi Mandy, This looks good! (not a reviewer) -- daniel On 10/9/13 12:29 PM, Mandy Chung wrote: Daniel, Thanks for the review. On 10/9/2013 12:38 AM, Daniel Fuchs wrote: This looks good - but I think you could move the changes line 554-562 and put them back inside the KnownLevel constructor where they were before. This would allow you to keep mirroredLevel final. That's right. I separated those lines in any earlier version and later added the private constructor per your suggestion. Small nit: the name 'custom' is a misnomer - as it will be true for both standard and custom levels... I renamed it to visible (visible levels can be found by Level.parse) Concerning the test I don't think you need to copy the property file to test.classes, because I believe jprt puts test.src inside the classpath. I think you meant jtreg. Thanks I have fixed that. (another possibility would be to use a custom subclass of ListResourceBundle instead) Finally, I think that test needs to be run in main/othervm mode - otherwise it might fail intermittently... Another good catch. Fixed. The updated webrev: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8026027/webrev.01/ Mandy
Re: RFR: 8024704: Improve API documentation of ClassLoader and ServiceLoader with respect to enumeration of resources.
On 09/10/2013 10:44, Daniel Fuchs wrote: Hi, Thank you all for the nice valuable feedback! I have updated the webrev with Alan's correction. The ClassLoader api note now reads: When overriding this method it is recommended that an implementation ensures that ... (and also fixed indentation) http://cr.openjdk.java.net/~dfuchs/webrev_8024704/webrev.01/ -- daniel Thanks for the update, this looks good to me. -Alan.
hg: jdk8/tl/jdk: 8008171: Refactor KeyStore.DomainLoadStoreParameter as a standalone class
Changeset: 91a752e3d50b Author:vinnie Date: 2013-10-09 10:48 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/91a752e3d50b 8008171: Refactor KeyStore.DomainLoadStoreParameter as a standalone class Reviewed-by: mullan, weijun + src/share/classes/java/security/DomainLoadStoreParameter.java ! src/share/classes/java/security/KeyStore.java ! src/share/classes/sun/security/provider/DomainKeyStore.java ! test/sun/security/provider/KeyStore/DKSTest.java
Re: Time to put a stop to Thread.stop?
On 09/10/2013 08:47, Dr Heinz M. Kabutz wrote: I noticed the other day when using Java 8 that Thread.stop(Throwable) was throwing an UOE. Unfortunately I missed the discussion in May amongst the thousands of other important JDK emails. Thus I presume that nothing I might have to say on the matter would change what is currently in the OpenJDK ;-) But I'd like to state it for the record anyway. Throwing checked exceptions in an unchecked context is, IMHO, something one should almost never do, except maybe in test cases. Up to now, I've known of quite a few ways to do this though: 1. Thread.stop(Throwable) with some mangling - see ThrowerConcurrent in http://www.javaspecialists.eu/archive/Issue144.html 2. Abusing generics 3. Class.newInstance(), described in Java Puzzlers (http://www.javaspecialists.eu/archive/Issue144.html) 4. Unsafe.throwException() similar to ThrowerConcurrent in #1. Since this is something that we should not do anyway, it is no harm to remove one of the ways, this being Thread.stop(Throwable). As I'm sure you can appreciate, it consumes a lot of oxygen to remove something and I'm hoping we don't have to spend any more time on this one. One comment on your list is that #1 differs in that it can cause other threads to misbehave and this is one of the main motives for finally pulling the plug on this method. For a sneakyThrow on the current thread then there are other options (as your puzzlers demonstrate) but hopefully there aren't too many cases where non-test code needs to do this. -Alan.
Re: RFR: 8025910 : (s) rename substream(long) - skip and substream(long, long) - slice
On 09/10/2013 04:38, Mike Duigou wrote: Hello all; Based upon feedback from the JavaOne Hands-On-Lab and other sources the 335 EG has decided to rename the two substream methods in the Streams interfaces to skip and slice. Webrev: http://cr.openjdk.java.net/~mduigou/JDK-8025910/0/webrev/ This looks okay to me and I assume the renames have caught any references in the javadoc. -Alan.
hg: jdk8/tl/langtools: 2 new changesets
Changeset: 0be3f1820e8b Author:jlahoda Date: 2013-10-09 13:06 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/0be3f1820e8b 8025141: java.lang.ClassFormatError: Illegal field modifiers in class (...) Summary: Should not generate non-public $assertionsDisabled field into interfaces Reviewed-by: jjg, vromero ! src/share/classes/com/sun/tools/javac/comp/Lower.java + test/tools/javac/defaultMethods/Assertions.java + test/tools/javac/defaultMethods/CannotChangeAssertionsStateAfterInitialized.java Changeset: 1b469fd31e35 Author:jlahoda Date: 2013-10-09 13:09 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/1b469fd31e35 8025087: Annotation processing api returns default modifier for interface static method Summary: ClassReader must not set Flags.DEFAULT for interface static methods Reviewed-by: vromero, jjg ! make/build.xml ! src/share/classes/com/sun/tools/javac/jvm/ClassReader.java ! src/share/classes/com/sun/tools/javac/resources/compiler.properties + test/tools/javac/defaultMethods/BadClassfile.java ! test/tools/javac/diags/examples.not-yet.txt + test/tools/javac/diags/examples/InvalidDefaultInterface/InvalidDefaultInterface.java + test/tools/javac/diags/examples/InvalidDefaultInterface/processors/CreateBadClassFile.java + test/tools/javac/diags/examples/InvalidStaticInterface/InvalidStaticInterface.java + test/tools/javac/diags/examples/InvalidStaticInterface/processors/CreateBadClassFile.java ! test/tools/javac/processing/model/element/TestExecutableElement.java
7011859: java/util/concurrent/Semaphore/RacingReleases.java failing
This test has been failing (very intermittently) for some time. It was assumed to be a HotSpot issue on SPARC but more recently it has been duplicated on Linux x64 too. David Simms has notes in the bug [1] from this instrumentation and analysis, and Doug has pushed a fix to his CVS that fixes the issue (although they may still be an underlying issue that still needs to be hunted down). Doug's fix updates AQS and AQLS to re-read the head and avoid the race. We need to bring this into jdk8/tl, the webrev with the patch is here: http://cr.openjdk.java.net/~alanb/7011859/webrev/ -Alan [1] https://bugs.openjdk.java.net/browse/JDK-7011859
Re: JDK 8 RFC 7189139 version 2: BigInteger's staticRandom field can be a source of bottlenecks
On Oct 8, 2013, at 10:05 PM, Brian Burkhalter brian.burkhal...@oracle.com
wrote:
On Oct 4, 2013, at 1:33 AM, Paul Sandoz wrote:
On Oct 4, 2013, at 9:18 AM, Aleksey Shipilev aleksey.shipi...@oracle.com
wrote:
On 10/04/2013 03:34 AM, Brian Burkhalter wrote:
Here is an alternative solution:http://cr.openjdk.java.net/~bpb/7189139.2/.
Seems OK with me,
Same here.
Based on previous comments by Aleksey and Paul I created this test
http://cr.openjdk.java.net/~bpb/7189139/PrimeTest.java
and ran it with the list of primes supplied by Aleksey, N=6000, and
certainty=100. This is the output:
---
Primes: /Users/bpb/Desktop/primes-50M.txt
N = 6000
certainty = 100
3562115 probable primes out of 3562115 candidates
Prime test = true
Not-prime test = true
Test succeeded!
---
Nice!
Perhaps as a separate bug you could place that code in the JDK test area as a
non-jtreg test?
Tip: you can use Files.lines().map(BigInteger::new) but we don't current have a
limitWhen operation so need to limit to N primes and not the content.
And... a bonus we can now go parallel and since TLR is used there is no longer
the contention point as with SR, which means we go faster!
I have added a modified version of the test code (see end of email) if you want
to play. If you place this test in the JDK test area i strongly recommend using
streams, it's a nice use-case we can point people to.
Here is the difference for the first 100 primes (with your patch applied)
for parallel and sequential execution (which also includes the fixed cost of
loading the primes from disk):
$ time java PrimeTest primes-50M.txt 100 100 true
Primes: primes-50M.txt
N = 100
certainty = 100
parallel = true
Loaded 100 primes
100 probable primes out of 100 candidates
Prime test = true
real0m16.252s
user1m59.065s
sys 0m0.649s
$ time java PrimeTest primes-50M.txt 100 100 false
Primes: primes-50M.txt
N = 100
certainty = 100
parallel = false
Loaded 100 primes
100 probable primes out of 100 candidates
Prime test = true
real0m54.613s
user1m0.278s
sys 0m0.343s
16s vs. 55s :-)
An updated webrev which differs only in having a correct Hg header is here:
http://cr.openjdk.java.net/~bpb/7189139/webrev.2/
If this looks good to go, would a Reviewer please issue an approval?
+1
Paul.
/*
* Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the Classpath exception as provided
* by Oracle in the LICENSE file that accompanied this code.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.math.BigInteger;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.HashSet;
import java.util.List;
import java.util.concurrent.atomic.LongAdder;
import java.util.stream.Stream;
import static java.util.stream.Collectors.toList;
public class PrimeTest {
public static void main(String[] args) throws Throwable {
File primesFile = new File(args[0]);
int upperBound = Integer.valueOf(args[1]);
int certainty = Integer.valueOf(args[2]);
boolean parallel = Boolean.valueOf(args[3]);
System.out.println(Primes: + primesFile + \nN = + upperBound +
\ncertainty = + certainty +
\nparallel = + parallel);
boolean primeTest = checkPrime(primesFile, upperBound, certainty,
parallel);
System.out.println(Prime test = + primeTest);
//boolean notPrimeTest = checkNotPrime(primesFile, upperBound,
certainty);
//System.out.println(Not-prime test = + notPrimeTest);
//
//if (!primeTest || !notPrimeTest) {
//throw new Exception(Test failed!);
//}
//System.out.println(Test succeeded!);
}
private static ListBigInteger parsePrimes(File
Re: RFR: 8025910 : (s) rename substream(long) - skip and substream(long, long) - slice
On Oct 9, 2013, at 5:38 AM, Mike Duigou mike.dui...@oracle.com wrote:
Hello all;
Based upon feedback from the JavaOne Hands-On-Lab and other sources the 335
EG has decided to rename the two substream methods in the Streams interfaces
to skip and slice.
Webrev:
http://cr.openjdk.java.net/~mduigou/JDK-8025910/0/webrev/
and the specdiff:
http://cr.openjdk.java.net/~mduigou/JDK-8025910/0/specdiff/overview-summary.html
Thanks,
Looks ok, but I have a preference to change the parameter name from
startInclusive to say n:
/**
* Returns a stream consisting of the remaining elements of this stream
* after discarding the first {@code n} elements of the stream.
* If this stream contains fewer than {@code n} elements then an
* empty stream will be returned.
*
* pThis is a a href=package-summary.html#StreamOpsstateful
* intermediate operation/a.
*
* @apiNote
* While {@code skip()} is generally a cheap operation on sequential
* stream pipelines, it can be quite expensive on ordered parallel
pipelines,
* especially for large values of {@code n}, since {@code skip(n)}
* is constrained to skip not just any emn/em elements, but the
* emfirst n/em elements in the encounter order. Using an unordered
* stream source (such as {@link #generate(Supplier)}) or removing the
* ordering constraint with {@link #unordered()} may result in significant
* speedups of {@code skip()} in parallel pipelines, if the semantics of
* your situation permit. If consistency with encounter order is required,
* and you are experiencing poor performance or memory utilization with
* {@code skip()} in parallel pipelines, switching to sequential execution
* with {@link #sequential()} may improve performance.
*
* @param n the number of leading elements to skip
* @return the new stream
* @throws IllegalArgumentException if {@code startInclusive} is negative
*/
StreamT skip(long n);
Paul.
Re: Review request for 8026027: Level.parse should return the custom Level instance instead of the mirrored Level
On 09/10/2013 11:41, Daniel Fuchs wrote: Hi Mandy, This looks good! +1 -Chris. (not a reviewer) -- daniel On 10/9/13 12:29 PM, Mandy Chung wrote: Daniel, Thanks for the review. On 10/9/2013 12:38 AM, Daniel Fuchs wrote: This looks good - but I think you could move the changes line 554-562 and put them back inside the KnownLevel constructor where they were before. This would allow you to keep mirroredLevel final. That's right. I separated those lines in any earlier version and later added the private constructor per your suggestion. Small nit: the name 'custom' is a misnomer - as it will be true for both standard and custom levels... I renamed it to visible (visible levels can be found by Level.parse) Concerning the test I don't think you need to copy the property file to test.classes, because I believe jprt puts test.src inside the classpath. I think you meant jtreg. Thanks I have fixed that. (another possibility would be to use a custom subclass of ListResourceBundle instead) Finally, I think that test needs to be run in main/othervm mode - otherwise it might fail intermittently... Another good catch. Fixed. The updated webrev: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8026027/webrev.01/ Mandy
hg: jdk8/tl/nashorn: 3 new changesets
Changeset: 8d29733ad609
Author:sundar
Date: 2013-10-09 10:47 +0200
URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/8d29733ad609
8026112: Function(with(x ? 1e81 : (x2.constructor = 0.1)){}) throws
AssertionError: double is not compatible with object
Reviewed-by: lagergren, hannesw
! src/jdk/nashorn/internal/codegen/CodeGenerator.java
+ test/script/basic/JDK-8026112.js
Changeset: 1e03d7caa68b
Author:sundar
Date: 2013-10-09 13:26 +0200
URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/1e03d7caa68b
8026125: Array.prototype.slice.call(Java.type(java.util.HashMap)) throws
ClassCastException: jdk.internal.dynalink.beans.StaticClass cannot be cast to
jdk.nashorn.internal.runtime.ScriptObject
Reviewed-by: hannesw, jlaskey
! src/jdk/nashorn/internal/objects/NativeArray.java
+ test/script/basic/JDK-8026125.js
Changeset: ec3094d9d5d5
Author:hannesw
Date: 2013-10-09 14:50 +0200
URL: http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/ec3094d9d5d5
8026008: Constant folding removes var statement
Reviewed-by: sundar, jlaskey
! src/jdk/nashorn/internal/codegen/FoldConstants.java
+ test/script/basic/JDK-8026008.js
+ test/script/basic/JDK-8026008.js.EXPECTED
hg: jdk8/tl/jdk: 8020061: Clarify reporting characteristics between splits
Changeset: 1cd20806fd5c Author:psandoz Date: 2013-10-09 15:19 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1cd20806fd5c 8020061: Clarify reporting characteristics between splits Reviewed-by: alanb, chegar ! src/share/classes/java/util/Spliterator.java
hg: jdk8/tl/jdk: 8026027: Level.parse should return the custom Level instance instead of the mirrored Level
Changeset: cf6e39cfdf50 Author:mchung Date: 2013-10-09 06:24 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/cf6e39cfdf50 8026027: Level.parse should return the custom Level instance instead of the mirrored Level Reviewed-by: dfuchs, chegar ! src/share/classes/java/util/logging/Level.java + test/java/util/logging/Level/CustomLevel.java + test/java/util/logging/Level/myresource.properties
Review Request for java.time test fix 8024612
Please Review a locale sensitive test correction: The API under test is based on Locale.getDefault(Locale.Category.FORMAT) and the test data should be using the same Locale. The failure was seen in locale: ar_EG with Locale.Category.FORMAT=ar_SA webrev: http://cr.openjdk.java.net/~rriggs/webrev-test-formatters-locale-8024612/ Thanks, Roger
Re: RFR: improved exception messages in java.time 8025718
Ping... any Reviewer... Thanks On 10/4/2013 3:37 PM, roger riggs wrote: Hi, Please review these small improvements in messages resulting from parsing date and time errors and corresponding tests. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-better-msg-8025718/ JBS: https://bugs.openjdk.java.net/browse/JDK-8025718 Thanks, Roger
Re: Review Request for java.time test fix 8024612
+1 On Oct 9, 2013, at 10:36 AM, roger riggs wrote: Please Review a locale sensitive test correction: The API under test is based on Locale.getDefault(Locale.Category.FORMAT) and the test data should be using the same Locale. The failure was seen in locale: ar_EG with Locale.Category.FORMAT=ar_SA webrev: http://cr.openjdk.java.net/~rriggs/webrev-test-formatters-locale-8024612/ Thanks, Roger Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: RFR: improved exception messages in java.time 8025718
+1 On Oct 9, 2013, at 10:38 AM, roger riggs wrote: Ping... any Reviewer... Thanks On 10/4/2013 3:37 PM, roger riggs wrote: Hi, Please review these small improvements in messages resulting from parsing date and time errors and corresponding tests. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-better-msg-8025718/ JBS: https://bugs.openjdk.java.net/browse/JDK-8025718 Thanks, Roger Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
hg: jdk8/tl/jdk: 2 new changesets
Changeset: e3b70e601c1c Author:rriggs Date: 2013-10-09 11:02 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e3b70e601c1c 8024612: java/time/tck/java/time/format/TCKDateTimeFormatters.java failed Summary: The test should be using the Locale.Category.FORMAT to verify the test data Reviewed-by: lancea ! test/java/time/tck/java/time/format/TCKDateTimeFormatters.java Changeset: 09e2a73aa1dc Author:rriggs Date: 2013-09-26 15:19 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/09e2a73aa1dc 8025718: Enhance error messages for parsing Summary: Add values and types to exception messages Reviewed-by: lancea Contributed-by: scolebou...@joda.org ! src/share/classes/java/time/DayOfWeek.java ! src/share/classes/java/time/Instant.java ! src/share/classes/java/time/LocalDate.java ! src/share/classes/java/time/LocalDateTime.java ! src/share/classes/java/time/LocalTime.java ! src/share/classes/java/time/Month.java ! src/share/classes/java/time/MonthDay.java ! src/share/classes/java/time/OffsetDateTime.java ! src/share/classes/java/time/OffsetTime.java ! src/share/classes/java/time/Year.java ! src/share/classes/java/time/YearMonth.java ! src/share/classes/java/time/ZoneId.java ! src/share/classes/java/time/ZoneOffset.java ! src/share/classes/java/time/ZonedDateTime.java ! src/share/classes/java/time/format/Parsed.java ! test/java/time/test/java/time/format/TestDateTimeFormatter.java
hg: jdk8/tl/jdk: 8023524: Mechanism to dump generated lambda classes / log lambda code generation
Changeset: c070001c4f60 Author:henryjen Date: 2013-10-09 09:41 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c070001c4f60 8023524: Mechanism to dump generated lambda classes / log lambda code generation Reviewed-by: plevart, mchung, forax, jjb Contributed-by: brian.go...@oracle.com, henry@oracle.com ! src/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java + src/share/classes/java/lang/invoke/ProxyClassesDumper.java + test/java/lang/invoke/lambda/LogGeneratedClassesTest.java
Slow readng tzdb.dat
I noticed recently that the JDK8 JVM was very slow starting on systems where
the JRE is on a high-latency, remote file system. I tracked this down to the
reading of tzdb.dat. In java/time/zone/TzdbZoneRulesProvider.java and
sun/util/calendar/ZoneInfoFile.java the tzdb.dat file is read using a
DataInputStream directly over a FileInputStream. Consequently there ends up
being a large number of very small (often a single byte) read requests to the
underlying O/S file system. This can be fixed trivially by adding a
BufferedInputStream between the DataInputStream and the FileInputStream.
Thus this:
try (DataInputStream dis = new DataInputStream(
new FileInputStream(new File(libDir, tzdb.dat {
becomes:
try (DataInputStream dis = new DataInputStream(
new BufferedInputStream(
new FileInputStream(new File(libDir, tzdb.dat)), 32000))) {
Tom Salter | Software Engineer | Java Middleware Development
Unisys | 2476 Swedesford Road | Malvern, PA 19355 | 610-648-2568 |
N385-2568
Re: RFR: 8023524: Mechanism to dump generated lambda classes / log lambda code generation
Thanks Mandy, and all others have reviewed and commented. Cheers, Henry On 10/09/2013 02:47 AM, Mandy Chung wrote: Hi Henry, On 10/8/2013 10:57 PM, Henry Jen wrote: Hi, Please review updated webrev at http://cr.openjdk.java.net/~henryjen/ccc/8023524/6/webrev ProxyClassesDumper looks simpler after moving the path validation to the static factory method. One minor comment: ProxyClassesDumper.getInstance returns null if the given path is invalid. For a null path, it can simply return null rather than throwing NPE. It's good to see the limited doPrivileged being used here. I can see that listing the limited permissions would be a tradeoff with maintainability and readability. This also leads to the question how one can determine the complete list of permissions required (besides testing) and guideline on when to use limited privileged and when to use the entire ACC (Jeff may have some thoughts). Anyway, just some comments related to limited doPrivileged and not anything related to this fix. Henry - your fix looks good for me. You don't not need to generate a new webrev if you make any change per my comment about null path unless others have any other comment. thanks Mandy
Re: RFR 8024709 : TreeMap.DescendingKeyIterator 'remove' confuses iterator position
On 10/9/13 3:12 AM, Alan Bateman wrote: On 08/10/2013 20:55, Brent Christian wrote: I've beefed up the test case Thanks for the update and expanding the test. I skimmed over the new test cases and they looks good. Thanks. There are a few commented out cases and it's not clear why this also. Ooops - I was confirming the new tests work using 5- and 3- element collections. I'll take the commented lines back out. Also a minor observation is that the method names are a bit inconsistent with the other method names in the test, they might be something to look at before pushing. Anything in particular? Other than clear(), I don't see other methods that take a collection and make it ready for testing. Most collections are populated inline, but my tests need a Map setup the exact same way many times, so I added prep[Map|Set]ForDescItrTests(). A lot of helper test methods are named checkXYZ, and that's how I named mine. The new tests check pretty specific behaviors, and I chose pretty descriptive names. I guess most existing method names are fully written out rather than abbreviated (e.g. checkNavigableMapKeys()), which I generally like to do, but mine got *really* long. I abbreviated the names to keep things under 80 characters without having to break everything onto multiple lines. I think this is ready for someone to push (I can send a changeset), unless more should be done with the method names in the test case. Thanks, -Brent
RFR (JAXP) : 8003262 reverse translation required changes
Hi, These resource bundles in JAXP were refactored due to request by WPTG. Now that the WPTG tool has been approved to support the original format, I'm reverting the previous change back to the original format: http://cr.openjdk.java.net/~joehw/jdk8/8003262/webrev/ Thanks, Joe
Re: Slow reading tzdb.dat
Hi Thomas,
Thanks for the report and suggestion, I filed an issue for it.
https://bugs.openjdk.java.net/browse/JDK-8026198
Roger
On 10/9/2013 1:08 PM, Salter, Thomas A wrote:
I noticed recently that the JDK8 JVM was very slow starting on systems where
the JRE is on a high-latency, remote file system. I tracked this down to the
reading of tzdb.dat. In java/time/zone/TzdbZoneRulesProvider.java and
sun/util/calendar/ZoneInfoFile.java the tzdb.dat file is read using a
DataInputStream directly over a FileInputStream. Consequently there ends up
being a large number of very small (often a single byte) read requests to the
underlying O/S file system. This can be fixed trivially by adding a
BufferedInputStream between the DataInputStream and the FileInputStream.
Thus this:
try (DataInputStream dis = new DataInputStream(
new FileInputStream(new File(libDir, tzdb.dat {
becomes:
try (DataInputStream dis = new DataInputStream(
new BufferedInputStream(
new FileInputStream(new File(libDir, tzdb.dat)), 32000))) {
Tom Salter | Software Engineer | Java Middleware Development
Unisys | 2476 Swedesford Road | Malvern, PA 19355 | 610-648-2568 |
N385-2568
Re: RFR (JAXP) : 8003262 reverse translation required changes
Hi Joe, I don't know what was the original format but these changes look reasonable. best regards, -- daniel (not a reviewer) On 10/9/13 7:35 PM, huizhe wang wrote: Hi, These resource bundles in JAXP were refactored due to request by WPTG. Now that the WPTG tool has been approved to support the original format, I'm reverting the previous change back to the original format: http://cr.openjdk.java.net/~joehw/jdk8/8003262/webrev/ Thanks, Joe
Re: RFR (JAXP) : 8003262 reverse translation required changes
+1 On Oct 9, 2013, at 1:35 PM, huizhe wang wrote: Hi, These resource bundles in JAXP were refactored due to request by WPTG. Now that the WPTG tool has been approved to support the original format, I'm reverting the previous change back to the original format: http://cr.openjdk.java.net/~joehw/jdk8/8003262/webrev/ Thanks, Joe Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: Slow readng tzdb.dat
May I suggest rather:
try (
File file = new File(libDir, tzdb.dat);
FileInputStream fis = new FileInputStream(file);
BufferedInputStream bis = new BufferedInputStream(fis, 32000);
DataInputStream dis = new DataInputStream(bis);
) {
...
That way, the resources are closed in the reverse order in which they
are opened and an exception in the middle of the creation chain does not
prevent the earlier resources from being closed.
Regards
Heinz
--
Dr Heinz M. Kabutz (PhD CompSci)
Author of The Java(tm) Specialists' Newsletter
Oracle Java Champion 2005-2013
JavaOne Rock Star Speaker 2012
http://www.javaspecialists.eu
Tel: +30 69 75 595 262
Skype: kabutz
Salter, Thomas A wrote:
I noticed recently that the JDK8 JVM was very slow starting on systems where
the JRE is on a high-latency, remote file system. I tracked this down to the
reading of tzdb.dat. In java/time/zone/TzdbZoneRulesProvider.java and
sun/util/calendar/ZoneInfoFile.java the tzdb.dat file is read using a
DataInputStream directly over a FileInputStream. Consequently there ends up
being a large number of very small (often a single byte) read requests to the
underlying O/S file system. This can be fixed trivially by adding a
BufferedInputStream between the DataInputStream and the FileInputStream.
Thus this:
try (DataInputStream dis = new DataInputStream(
new FileInputStream(new File(libDir, tzdb.dat {
becomes:
try (DataInputStream dis = new DataInputStream(
new BufferedInputStream(
new FileInputStream(new File(libDir, tzdb.dat)), 32000))) {
Tom Salter | Software Engineer | Java Middleware Development
Unisys | 2476 Swedesford Road | Malvern, PA 19355 | 610-648-2568 | N385-2568
Re: RFR 8024709 : TreeMap.DescendingKeyIterator 'remove' confuses iterator position
On Oct 9 2013, at 10:33 , Brent Christian wrote: On 10/9/13 3:12 AM, Alan Bateman wrote: On 08/10/2013 20:55, Brent Christian wrote: I've beefed up the test case Thanks for the update and expanding the test. I skimmed over the new test cases and they looks good. Thanks. There are a few commented out cases and it's not clear why this also. Ooops - I was confirming the new tests work using 5- and 3- element collections. I'll take the commented lines back out. Also a minor observation is that the method names are a bit inconsistent with the other method names in the test, they might be something to look at before pushing. Anything in particular? Other than clear(), I don't see other methods that take a collection and make it ready for testing. Most collections are populated inline, but my tests need a Map setup the exact same way many times, so I added prep[Map|Set]ForDescItrTests(). A lot of helper test methods are named checkXYZ, and that's how I named mine. The new tests check pretty specific behaviors, and I chose pretty descriptive names. I guess most existing method names are fully written out rather than abbreviated (e.g. checkNavigableMapKeys()), which I generally like to do, but mine got *really* long. I abbreviated the names to keep things under 80 characters without having to break everything onto multiple lines. I think this is ready for someone to push (I can send a changeset), unless more should be done with the method names in the test case. I haven't looked at the latest webrev but if you send me a changeset I will review it and push it. Mike
Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown
On Oct 8, 2013, at 2:43 AM, Alan Bateman wrote:
Thanks for the previous comments. My feeling at this point is to do one of
two things:
A) defer to something after JDK 8, or
B) on EAI_AGAIN do not retry but set the cause of the UAE to new
SomeException(gai_strerror(error)) where SomeException could be for example
Exception, RuntimeException, or IOException.
Comments?
Throwing UHE with a useful message or cause would be best, so option B.
However, I don't think it is critical for jdk8 as it's not a new issue (at
least I think UHE has always been thrown with just the hostname, never the
reason if it is known). So if there isn't time to be confident with the patch
then it should be okay to move this out to 9.
I have created a simple implementation for option B:
http://cr.openjdk.java.net/~bpb/8010371/webrev.3/
I should note that the Unix Inet6 implementation was already using the call
ThrowUnknownHostExceptionWithGaiError() to generate the UHE so in this case the
message should already have been useful. This call formats the UHE message such
as would:
sprintf(error_message, %s: %s, hostname, gai_strerror(error))
I changed the Unix Inet4 implementation to do this as well instead of calling
JNU_ThrowByName().
For Windows I added a similar NET_ ThrowUnknownHostExceptionWithGaiError and
modified Inet{4,6} to mimic the Unix case. Note that the Windows code has not
yet been compiled pending comments.
Thanks,
Brian
RFR: 7044282: (reflect) Class.forName and Array.newInstance are inconsistent regarding multidimensional arrays
Hi Please review this spec update and test for getting array classes and instances of more dimensions than the class file can express or the VM can handle. Array.newInstance have a test for arrays of more dimensions than 255, this patch adds a test for Class.forName as well. Also the javadoc for Array.newInstance are clarified. Bug: https://bugs.openjdk.java.net/browse/JDK-7044282 Webrev: http://cr.openjdk.java.net/~jfranck/7044282/webrev.00/ cheers /Joel
Re: JDK 8 code review request forJDK-8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation
On Oct 9, 2013, at 3:56 AM, Joe Darcy joe.da...@oracle.com wrote:
Hello,
Please review the patch below which addresses
JDK-8024354 Explicitly permit DoubleStream.sum()/average()
implementations to use higher precision summation
Thanks,
The note feels more like an api note since it leaves me wondering what does the
current implementation does. Presumably we should mention that current impl
does do not do anything special?
Paul.
-Joe
diff -r f1e31376f419 src/share/classes/java/util/DoubleSummaryStatistics.java
--- a/src/share/classes/java/util/DoubleSummaryStatistics.java Wed Oct 09
00:10:02 2013 +0100
+++ b/src/share/classes/java/util/DoubleSummaryStatistics.java Tue Oct 08
18:54:55 2013 -0700
@@ -118,6 +118,11 @@
* value is a {@code NaN} or the sum is at any point a {@code NaN} then
the
* sum will be {@code NaN}.
*
+ * @implNote This method may be implemented using compensated
+ * summation or other technique to reduce the error bound in the
+ * numerical sum compared to a simple summation of {@code double}
+ * values.
+ *
* @return the sum of values, or zero if none
*/
public final double getSum() {
@@ -161,6 +166,10 @@
* value is a {@code NaN} or the sum is at any point a {@code NaN} then
the
* average will be {@code NaN}.
*
+ * @implNote This method may be implemented using compensated
+ * summation or other technique to reduce the error bound in the
+ * numerical sum used to compute the average.
+ *
* @return the arithmetic mean of values, or zero if none
*/
public final double getAverage() {
hg: jdk8/tl/jdk: 8024076: Incorrect 2 - 4 year parsing and resolution in DateTimeFormatter
Changeset: d42fe440bda8 Author:rriggs Date: 2013-10-09 13:34 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d42fe440bda8 8024076: Incorrect 2 - 4 year parsing and resolution in DateTimeFormatter Summary: Add appendValueReduced method based on a ChronoLocalDate to provide context for the value Reviewed-by: sherman Contributed-by: scolebou...@joda.org ! src/share/classes/java/time/format/DateTimeFormatterBuilder.java ! test/java/time/tck/java/time/format/TCKDateTimeFormatterBuilder.java ! test/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java ! test/java/time/test/java/time/format/TestReducedParser.java ! test/java/time/test/java/time/format/TestReducedPrinter.java
RFR: 8022718 : Runtime accessibility checking: protected class, if extended, should be accessible from another package
bug: https://bugs.openjdk.java.net/browse/JDK-8022718 webrev: http://cr.openjdk.java.net/~drchase/8022718/webrev.00/ Problem: Needed implementation for change to the spec for JVM behavior (from the bug report): JSR 335 spec, chapter 15.28.2 Run-time Evaluation of Method References contains following assertion: If the method reference has the form ExpressionName :: NonWildTypeArgumentsopt Identifier or Primary :: NonWildTypeArgumentsopt Identifier, the body of the invocation method has the effect of invoking the compile-time declaration of the method reference, as described in 15.12.4.3, 15.12.4.4, 15.12.4.5.[jsr335-15.28.2-41] This assertions refers to chapter 15.12.4.3. Check Accessibility of Type and Method which contains following assertions: Let C be the class containing the method invocation, and let T be the qualifying type of the method invocation (§13.1), and let m be the name of the method as determined at compile-time (§15.12.3). ... If T is in a different package than C, and T is protected, then T is accessible if and only if C is a subclass of T. ... Note that this cannot be expressed in compatibly compiled Java (yet). Testing requires either monkeying with change and recompilation, or doctoring bytecodes at runtime. Fix: Turns out that all that was needed was the missing case of checking the conditions for protected access, despite suggestions in the bug report that something different might be needed (the different version of the access flags was not suitable, and not using them makes for a smaller change anyway). The new behavior needed to be mentioned in the Javadoc, so I did. Testing: 1) wrote new self-contained test to verify behavior (*) 2) jtreg of jdk/lambda java/lang/invoke java/util/stream (passed) 3) running ute on vm.quick.testlist 4) running JPRT on testset core (well, trying to run JPRT). Issues: - It's highly likely I botched the Javadoc changes; I'm sure we have standards, and I'm sure I don't know them. - (*) The bug, as reported, describes as wrong the behavior obtained if the -PUBLIC +PROTECTED access mode appears on the innerclass attributes, not on the class attributes. I explored this space while developing the test, and if the class itself is marked protected, the failure occurs much earlier in the game when an attempt is made to load the subclass. The test is written to mimic standalone (with bytecode rewriting) what was seen in the test. - I decided to put the test in its own subdirectory of test/java/lang/invoke with its own informative name, and not directly in that directory, because all these invocation/protection tests have overlapping and near-overlapping class names, and so it would be confusing not to do this. Unfortunately, that means there's now two copies of the BogoLoader checked in to the test tree. David
hg: jdk8/tl/jdk: 8016252: More defensive HashSet.readObject
Changeset: b86e6700266e Author:bpb Date: 2013-10-09 11:47 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b86e6700266e 8016252: More defensive HashSet.readObject Summary: Add data validation checks in readObject(). Reviewed-by: alanb, mduigou, chegar Contributed-by: Brian Burkhalter brian.burkhal...@oracle.com ! src/share/classes/java/util/HashSet.java + test/java/util/HashSet/Serialization.java
RFR: JDK-8025712,,(props) Possible memory leak in java_props_md.c / ParseLocale
Hi All, This fix is to solve the memory leak issue in ParseLocale() function of jdk/src/solaris/native/java/lang/java_props_md.c. Because the locale, lc, is copied into temp, it is not necessary to do the strdup(), which leads to the memery leak. The fix simply removes the line of strdup operation. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8025712/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8025712 -Dan
Re: JDK 8 RFR 8016252: More defensive HashSet.readObject
On Oct 9, 2013, at 3:41 AM, Alan Bateman wrote: On 08/10/2013 21:50, Brian Burkhalter wrote: I have updated the webrev accordingly http://cr.openjdk.java.net/~bpb/8016252/webrev.3/ This looks good, I think this gets us to where we wanted to be. Pushed: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b86e6700266e : I skimmed over the test but it doesn't appear to exercise anything new. If you want to exercise the checks then it would require deserializing from a byte stream that results in bad loadFactor, size and capacity values. It might not be worth it of course. No, it does not exercise anything new. I actually did try inserting random garbage into the written-out byte stream, but without knowing where to do so to affect the fields of interest it is rather useless and causes totally unpredictable results. I don't know that this test really needs to be included with the patch. There are tools around for decoding serialization streams and in this case you need to change the right bytes to exercise the conditions. It's probably not worth spending too much time on this (there are more important things to get done before ZBB) so I wouldn't object to just creating another bug to improve the test coverage for this area and move on. I created https://bugs.openjdk.java.net/browse/JDK-8026216 so I will move on from this topic. Thanks, Brian
Re: Slow readng tzdb.dat
On 09/10/2013 19:00, Dr Heinz M. Kabutz wrote:
That way, the resources are closed in the reverse order in which they
are opened and an exception in the middle of the creation chain does not
prevent the earlier resources from being closed.
But there is only one resource.
File tzdb = new File(libDir, tzdb.dat);
try (InputStream rawIn = new FileInputStream(tzdb)) {
DataInputStream dis = new DataInputStream(
new BufferedInputStream(rawIn, 32000)
);
Tom
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 9, 2013, at 1:56 AM, Chris Hegarty wrote: This latest webrev looks good to me. If you haven't already got a sponsor, then I would be happy to sponsor this for you. Thanks. Since there are minor spec clarifications, to document long standing behavior, then a CCC request will need to be submitted, and approved, before integration. Yes, I intend to do that. I was hoping to get a consensus on the review first but I will file the request this week in any case. Thanks, Brian
Re: RFR: 8022718 : Runtime accessibility checking: protected class, if extended, should be accessible from another package
On Oct 9, 2013, at 11:49 AM, David Chase david.r.ch...@oracle.com wrote: bug: https://bugs.openjdk.java.net/browse/JDK-8022718 webrev: http://cr.openjdk.java.net/~drchase/8022718/webrev.00/ Problem: Needed implementation for change to the spec for JVM behavior (from the bug report): JSR 335 spec, chapter 15.28.2 Run-time Evaluation of Method References contains following assertion: If the method reference has the form ExpressionName :: NonWildTypeArgumentsopt Identifier or Primary :: NonWildTypeArgumentsopt Identifier, the body of the invocation method has the effect of invoking the compile-time declaration of the method reference, as described in 15.12.4.3, 15.12.4.4, 15.12.4.5.[jsr335-15.28.2-41] This assertions refers to chapter 15.12.4.3. Check Accessibility of Type and Method which contains following assertions: Let C be the class containing the method invocation, and let T be the qualifying type of the method invocation (§13.1), and let m be the name of the method as determined at compile-time (§15.12.3). ... If T is in a different package than C, and T is protected, then T is accessible if and only if C is a subclass of T. ... JSR 292 doesn't implement the Java language spec; it implements the JVM spec. In this case there is a difference. Mixing language and JVM spec. can lead to confusion down the road. The error is that Class.getModifiers (which is correctly saying protected) is information about the source code. The JVM does not look at this for checking access; it looks at the more obscure Reflection.getClassAccessFlags. Those flags will say public (correctly) for a class compiled as protected. In order to align exactly with JVM behavior (not Java semantics) the Reflection.getClassAccessFlags need to be looked at. So I think this fix is wrong. I think the root cause of this problem was me reaching for Class.getModifiers when I should have grabbed Reflection.getClassAccessFlags. About BogoLoader: I would prefer to see it hoisted into a place where more tests can use it. Consider test/java/lang/invoke/indify/Indify.java as a model; name it test/java/lang/invoke/loader/BogoLoader.java. — John Note that this cannot be expressed in compatibly compiled Java (yet). Testing requires either monkeying with change and recompilation, or doctoring bytecodes at runtime. Fix: Turns out that all that was needed was the missing case of checking the conditions for protected access, despite suggestions in the bug report that something different might be needed (the different version of the access flags was not suitable, and not using them makes for a smaller change anyway). The new behavior needed to be mentioned in the Javadoc, so I did. Testing: 1) wrote new self-contained test to verify behavior (*) 2) jtreg of jdk/lambda java/lang/invoke java/util/stream (passed) 3) running ute on vm.quick.testlist 4) running JPRT on testset core (well, trying to run JPRT). Issues: - It's highly likely I botched the Javadoc changes; I'm sure we have standards, and I'm sure I don't know them. - (*) The bug, as reported, describes as wrong the behavior obtained if the -PUBLIC +PROTECTED access mode appears on the innerclass attributes, not on the class attributes. I explored this space while developing the test, and if the class itself is marked protected, the failure occurs much earlier in the game when an attempt is made to load the subclass. The test is written to mimic standalone (with bytecode rewriting) what was seen in the test. - I decided to put the test in its own subdirectory of test/java/lang/invoke with its own informative name, and not directly in that directory, because all these invocation/protection tests have overlapping and near-overlapping class names, and so it would be confusing not to do this. Unfortunately, that means there's now two copies of the BogoLoader checked in to the test tree. David
Re: RFR: JDK-8025712, , (props) Possible memory leak in java_props_md.c / ParseLocale
Looks good to me. Naoto On 10/9/13 11:51 AM, Dan Xu wrote: Hi All, This fix is to solve the memory leak issue in ParseLocale() function of jdk/src/solaris/native/java/lang/java_props_md.c. Because the locale, lc, is copied into temp, it is not necessary to do the strdup(), which leads to the memery leak. The fix simply removes the line of strdup operation. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8025712/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8025712 -Dan
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 9, 2013, at 3:06 AM, Michael McMahon wrote:
If I read the code correctly then the long standing behavior of
URLClassLoader.getPermissions(null) was to throw a NPE, now it is changed to
return the empty collection in order to match the super type
(SecureClassLoader). I can't think of anything that would break so this is
probably okay, just maybe a bit surprising.
I'm still not sure about that change. Its effect in sub-classes will be to
continue on in the sub-class implementation
of getPermissions() with a null codesource parameter, where currently NPE
would be thrown.
Well, upon re-examination I actually agree with you, especially considering the
class hierarchy:
AppletClassLoader :: URLClassLoader :: SecureClassLoader :: ClassLoader
I have posted an updated webrev here:
http://cr.openjdk.java.net/~bpb/7179567/webrev.4/
The differences with respect to the previous webrev [1] are:
1) the javadoc change of SecureClassLoader has been reverted;
2) these statements
665 if (codesource == null) {
666 return perms;
667 }
668
in URLClassLoader.getPermissions() have been reverted;
3) the javadoc of getPermissions() in URLClassLoader and AppletClassLoader has
been updated to add @exception NullPointerException if {@code codesource} is
{@code null}.
The CCC request to be associated with this change would indicate that
@exception NullPointerException has been added to the javadoc of:
A) all public URLClassLoader constructors;
B) URLClassLoader.findClass();
C) URLClassLoader.getPermissions();
D) both URLClassLoader.newInstance() methods;
E) AppletClassLoader.getPermissions().
The only actual *code* changes are in URLClassLoader.
Thanks,
Brian
[1] http://cr.openjdk.java.net/~bpb/7179567/webrev.3/
Re: RFR: 8022718 : Runtime accessibility checking: protected class, if extended, should be accessible from another package
How about this one? http://cr.openjdk.java.net/~drchase/8022718/webrev.01/ This version tests both versions of the modifiers for public -- ref.getModifiers apparently sets public in some cases where the other one does not. Not 100% sure what those cases are, I suspect arrays from the cryptic failure message. Retested so far on the added test, and on j/l/i/MethodHandlesTest, which has been a fruitful source of problems so far. David On 2013-10-09, at 3:22 PM, John Rose john.r.r...@oracle.com wrote: JSR 292 doesn't implement the Java language spec; it implements the JVM spec. In this case there is a difference. Mixing language and JVM spec. can lead to confusion down the road. The error is that Class.getModifiers (which is correctly saying protected) is information about the source code. The JVM does not look at this for checking access; it looks at the more obscure Reflection.getClassAccessFlags. Those flags will say public (correctly) for a class compiled as protected. In order to align exactly with JVM behavior (not Java semantics) the Reflection.getClassAccessFlags need to be looked at. So I think this fix is wrong. I think the root cause of this problem was me reaching for Class.getModifiers when I should have grabbed Reflection.getClassAccessFlags. About BogoLoader: I would prefer to see it hoisted into a place where more tests can use it. Consider test/java/lang/invoke/indify/Indify.java as a model; name it test/java/lang/invoke/loader/BogoLoader.java. — John Note that this cannot be expressed in compatibly compiled Java (yet). Testing requires either monkeying with change and recompilation, or doctoring bytecodes at runtime. Fix: Turns out that all that was needed was the missing case of checking the conditions for protected access, despite suggestions in the bug report that something different might be needed (the different version of the access flags was not suitable, and not using them makes for a smaller change anyway). The new behavior needed to be mentioned in the Javadoc, so I did. Testing: 1) wrote new self-contained test to verify behavior (*) 2) jtreg of jdk/lambda java/lang/invoke java/util/stream (passed) 3) running ute on vm.quick.testlist 4) running JPRT on testset core (well, trying to run JPRT). Issues: - It's highly likely I botched the Javadoc changes; I'm sure we have standards, and I'm sure I don't know them. - (*) The bug, as reported, describes as wrong the behavior obtained if the -PUBLIC +PROTECTED access mode appears on the innerclass attributes, not on the class attributes. I explored this space while developing the test, and if the class itself is marked protected, the failure occurs much earlier in the game when an attempt is made to load the subclass. The test is written to mimic standalone (with bytecode rewriting) what was seen in the test. - I decided to put the test in its own subdirectory of test/java/lang/invoke with its own informative name, and not directly in that directory, because all these invocation/protection tests have overlapping and near-overlapping class names, and so it would be confusing not to do this. Unfortunately, that means there's now two copies of the BogoLoader checked in to the test tree. David
hg: jdk8/tl/jdk: 5 new changesets
Changeset: 1597066b58ee Author:valeriep Date: 2013-10-08 11:07 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1597066b58ee 7196382: PKCS11 provider should support 2048-bit DH Summary: Query and enforce range checking using the values from native PKCS11 library. Reviewed-by: xuelei ! src/share/classes/com/sun/crypto/provider/DHParameterGenerator.java ! src/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java + test/sun/security/pkcs11/KeyPairGenerator/TestDH2048.java Changeset: 3da8be8d13bf Author:valeriep Date: 2013-10-08 11:17 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3da8be8d13bf 8012900: CICO ignores AAD in GCM mode Summary: Change GCM decryption to not return result until tag verification passed Reviewed-by: xuelei ! src/share/classes/com/sun/crypto/provider/CipherBlockChaining.java ! src/share/classes/com/sun/crypto/provider/CipherCore.java ! src/share/classes/com/sun/crypto/provider/CipherFeedback.java ! src/share/classes/com/sun/crypto/provider/CounterMode.java ! src/share/classes/com/sun/crypto/provider/ElectronicCodeBook.java ! src/share/classes/com/sun/crypto/provider/FeedbackCipher.java ! src/share/classes/com/sun/crypto/provider/GCTR.java ! src/share/classes/com/sun/crypto/provider/GaloisCounterMode.java ! src/share/classes/com/sun/crypto/provider/OutputFeedback.java ! src/share/classes/com/sun/crypto/provider/PCBC.java ! src/share/classes/javax/crypto/CipherSpi.java + test/com/sun/crypto/provider/Cipher/AES/TestCICOWithGCMAndAAD.java Changeset: f4305254f92f Author:valeriep Date: 2013-10-08 11:35 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f4305254f92f 8014374: Cannot initialize AES/GCM/NoPadding on wrap/unseal on solaris with OracleUcrypto Summary: Removed OracleUcrypto provider regression tests from OpenJDK Reviewed-by: xuelei - test/com/oracle/security/ucrypto/TestAES.java - test/com/oracle/security/ucrypto/TestDigest.java - test/com/oracle/security/ucrypto/TestRSA.java - test/com/oracle/security/ucrypto/UcryptoTest.java Changeset: e044b0151858 Author:valeriep Date: 2013-10-08 14:41 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e044b0151858 8025967: addition of -Werror broke the old build Summary: Fixed and suppressed compiler warnings on rawtypes Reviewed-by: vinnie ! src/share/classes/com/sun/jndi/ldap/LdapCtxFactory.java ! src/share/classes/com/sun/jndi/ldap/LdapPoolManager.java ! src/share/classes/com/sun/net/ssl/internal/www/protocol/https/HttpsURLConnectionOldImpl.java ! src/share/classes/java/lang/instrument/Instrumentation.java ! src/share/classes/java/net/ContentHandler.java ! src/share/classes/javax/crypto/JceSecurityManager.java ! src/share/classes/sun/instrument/InstrumentationImpl.java ! src/share/classes/sun/net/www/content/image/gif.java ! src/share/classes/sun/net/www/content/image/jpeg.java ! src/share/classes/sun/net/www/content/image/png.java ! src/share/classes/sun/net/www/content/image/x_xbitmap.java ! src/share/classes/sun/net/www/content/image/x_xpixmap.java ! src/share/classes/sun/net/www/protocol/https/HttpsURLConnectionImpl.java ! src/share/classes/sun/reflect/misc/MethodUtil.java ! src/share/classes/sun/security/provider/AuthPolicyFile.java ! src/share/classes/sun/security/provider/SubjectCodeSource.java ! src/share/classes/sun/security/tools/jarsigner/Main.java ! src/share/classes/sun/security/tools/keytool/Main.java ! src/share/classes/sun/security/tools/policytool/PolicyTool.java Changeset: 7a7b73a40bb1 Author:valeriep Date: 2013-10-09 13:07 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7a7b73a40bb1 Merge - src/share/classes/com/sun/jdi/connect/package.html - src/share/classes/com/sun/jdi/connect/spi/package.html - src/share/classes/com/sun/jdi/event/package.html - src/share/classes/com/sun/jdi/package.html - src/share/classes/com/sun/jdi/request/package.html - src/share/classes/com/sun/management/package.html - src/share/classes/com/sun/tools/attach/package.html - src/share/classes/com/sun/tools/attach/spi/package.html - src/share/classes/com/sun/tools/jconsole/package.html
Re: Slow readng tzdb.dat
Tom Hawtin wrote:
On 09/10/2013 19:00, Dr Heinz M. Kabutz wrote:
That way, the resources are closed in the reverse order in which they
are opened and an exception in the middle of the creation chain does not
prevent the earlier resources from being closed.
But there is only one resource.
File tzdb = new File(libDir, tzdb.dat);
try (InputStream rawIn = new FileInputStream(tzdb)) {
DataInputStream dis = new DataInputStream(
new BufferedInputStream(rawIn, 32000)
);
But you've written it slightly differently from the original
try (DataInputStream dis = new DataInputStream(
new FileInputStream(new File(libDir, tzdb.dat {
Your code is also fine, as it isolates the resource that should be
closed should something go wrong during the initialization of the
DataInputStream / BufferedInputStream. Either of our solutions would be
good and it would just be a matter of how you'd like to scope the
various local variables.
Re: JDK 8 code review request forJDK-8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation
On 10/09/2013 11:36 AM, Paul Sandoz wrote: On Oct 9, 2013, at 3:56 AM, Joe Darcy joe.da...@oracle.com wrote: Hello, Please review the patch below which addresses JDK-8024354 Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation Thanks, The note feels more like an api note since it leaves me wondering what does the current implementation does. Presumably we should mention that current impl does do not do anything special? The goal here is to explicitly allow (but not require) better-than-straightforard summation algorithms to be used. Time permitting, I'd prefer to get a better implementation pushed before JDK 8 GA and, failing that, in an 8 update before JDK 9. -Joe
RFR: Lambda 8026213: Reflection support for private methods in interfaces
Please review: webrev: http://cr.openjdk.java.net/~acorn/8026213/webrev/ bug: https://bugs.openjdk.java.net/browse/JDK-8026213 Summary: Reflection generates code dynamically to speed up reflection processing after startup. The first 15 runs of a reflection call use the vm code path, after that we use the generated code path, which needs to use invokespecial on private methods in interfaces. Tested: Test attached to the bug Also - all the 8011311 private method testing was run with this in the build: Robert Field's TypeTest 8025475 test defmeth privatemethodstest with reflection John Rose's intfbug jtreg: java.util, java.lang jck vm, lang thanks, Karen
Re: JDK 8 code review request forJDK-8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation
On 10/08/2013 08:08 PM, Mike Duigou wrote:
This seems to contradict the main documentation for these methods. Perhaps instead we
should remove the The average returned can vary depending upon the order in which
values are recorded. This is due to accumulated rounding error in addition of values of
differing magnitudes. Values sorted by increasing absolute magnitude tend to yield more
accurate results. into an @implNote?
I also suspect that having this documentation only in DoubleSummaryStatistics
may be too hidden away. Perhaps similar docs on DoubleStream sum() and
average() methods as well?
Hello,
Please review the second take on this change which is expanded to
include DoubleStream.sum:
http://cr.openjdk.java.net/~darcy/8024354.1/
Patch below,
Thanks,
-Joe
--- old/src/share/classes/java/util/DoubleSummaryStatistics.java
2013-10-09 16:37:23.0 -0700
+++ new/src/share/classes/java/util/DoubleSummaryStatistics.java
2013-10-09 16:37:23.0 -0700
@@ -111,12 +111,24 @@
/**
* Returns the sum of values recorded, or zero if no values have been
- * recorded. The sum returned can vary depending upon the order in
which
- * values are recorded. This is due to accumulated rounding error in
- * addition of values of differing magnitudes. Values sorted by
increasing
- * absolute magnitude tend to yield more accurate results. If any
recorded
- * value is a {@code NaN} or the sum is at any point a {@code NaN}
then the
- * sum will be {@code NaN}.
+ * recorded.
+ *
+ * If any recorded value is a NaN or the sum is at any point a NaN
+ * then the sum will be NaN.
+ *
+ * @apiNote The value of a floating-point sum is a function both
+ * of the input values as well as the order of addition
+ * operations. The order of addition operations of this method is
+ * intentionally not defined to allow for implementation flexibility
+ * to improve the speed and accuracy of the computed result.
+ *
+ * In particular, this method may be implemented using compensated
+ * summation or other technique to reduce the error bound in the
+ * numerical sum compared to a simple summation of {@code double}
+ * values.
+ *
+ * Sorting values by increasing absolute magnitude tends to yield
+ * more accurate results.
*
* @return the sum of values, or zero if none
*/
@@ -153,13 +165,21 @@
}
/**
- * Returns the arithmetic mean of values recorded, or zero if no
values have been
- * recorded. The average returned can vary depending upon the order in
- * which values are recorded. This is due to accumulated rounding
error in
- * addition of values of differing magnitudes. Values sorted by
increasing
- * absolute magnitude tend to yield more accurate results. If any
recorded
- * value is a {@code NaN} or the sum is at any point a {@code NaN}
then the
- * average will be {@code NaN}.
+ * Returns the arithmetic mean of values recorded, or zero if no
+ * values have been recorded.
+ *
+ * If any recorded value is a NaN or the sum is at any point a NaN
+ * then the average will be code NaN.
+ *
+ * @apiNote The average returned can vary depending upon the order in
+ * which values are recorded.
+ *
+ * This method may be implemented using compensated summation or
+ * other technique to reduce the error bound in the numerical sum
+ * used to compute the average.
+ *
+ * Values sorted by increasing absolute magnitude tend to yield
+ * more accurate results.
*
* @return the arithmetic mean of values, or zero if none
*/
--- old/src/share/classes/java/util/stream/DoubleStream.java 2013-10-09
16:37:23.0 -0700
+++ new/src/share/classes/java/util/stream/DoubleStream.java 2013-10-09
16:37:23.0 -0700
@@ -502,22 +502,42 @@
BiConsumerR, R combiner);
/**
- * Returns the sum of elements in this stream. The sum returned
can vary
- * depending upon the order in which elements are encountered. This
is due
- * to accumulated rounding error in addition of values of differing
- * magnitudes. Elements sorted by increasing absolute magnitude tend to
- * yield more accurate results. If any stream element is a {@code
NaN} or
- * the sum is at any point a {@code NaN} then the sum will be
{@code NaN}.
- * This is a special case of a
- * a href=package-summary.html#Reductionreduction/a and is
+ * Returns the sum of elements in this stream.
+ *
+ * Summation is a special case of a a
+ * href=package-summary.html#Reductionreduction/a. If
+ * floating-point summation were exact, this method would be
* equivalent to:
+ *
* pre{@code
* return reduce(0, Double::sum);
* }/pre
*
+ * However, since floating-point summation is not exact, the above
+ *
Re: JDK 8 code review request forJDK-8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation
Big improvement. This looks like the right direction. DoubleStream.average()
needs the same treatment for completeness.
Mike
On Oct 9 2013, at 16:38 , Joe Darcy wrote:
On 10/08/2013 08:08 PM, Mike Duigou wrote:
This seems to contradict the main documentation for these methods. Perhaps
instead we should remove the The average returned can vary depending upon
the order in which values are recorded. This is due to accumulated rounding
error in addition of values of differing magnitudes. Values sorted by
increasing absolute magnitude tend to yield more accurate results. into an
@implNote?
I also suspect that having this documentation only in
DoubleSummaryStatistics may be too hidden away. Perhaps similar docs on
DoubleStream sum() and average() methods as well?
Hello,
Please review the second take on this change which is expanded to include
DoubleStream.sum:
http://cr.openjdk.java.net/~darcy/8024354.1/
Patch below,
Thanks,
-Joe
--- old/src/share/classes/java/util/DoubleSummaryStatistics.java 2013-10-09
16:37:23.0 -0700
+++ new/src/share/classes/java/util/DoubleSummaryStatistics.java 2013-10-09
16:37:23.0 -0700
@@ -111,12 +111,24 @@
/**
* Returns the sum of values recorded, or zero if no values have been
- * recorded. The sum returned can vary depending upon the order in which
- * values are recorded. This is due to accumulated rounding error in
- * addition of values of differing magnitudes. Values sorted by
increasing
- * absolute magnitude tend to yield more accurate results. If any
recorded
- * value is a {@code NaN} or the sum is at any point a {@code NaN} then
the
- * sum will be {@code NaN}.
+ * recorded.
+ *
+ * If any recorded value is a NaN or the sum is at any point a NaN
+ * then the sum will be NaN.
+ *
+ * @apiNote The value of a floating-point sum is a function both
+ * of the input values as well as the order of addition
+ * operations. The order of addition operations of this method is
+ * intentionally not defined to allow for implementation flexibility
+ * to improve the speed and accuracy of the computed result.
+ *
+ * In particular, this method may be implemented using compensated
+ * summation or other technique to reduce the error bound in the
+ * numerical sum compared to a simple summation of {@code double}
+ * values.
+ *
+ * Sorting values by increasing absolute magnitude tends to yield
+ * more accurate results.
*
* @return the sum of values, or zero if none
*/
@@ -153,13 +165,21 @@
}
/**
- * Returns the arithmetic mean of values recorded, or zero if no values
have been
- * recorded. The average returned can vary depending upon the order in
- * which values are recorded. This is due to accumulated rounding error
in
- * addition of values of differing magnitudes. Values sorted by
increasing
- * absolute magnitude tend to yield more accurate results. If any
recorded
- * value is a {@code NaN} or the sum is at any point a {@code NaN} then
the
- * average will be {@code NaN}.
+ * Returns the arithmetic mean of values recorded, or zero if no
+ * values have been recorded.
+ *
+ * If any recorded value is a NaN or the sum is at any point a NaN
+ * then the average will be code NaN.
+ *
+ * @apiNote The average returned can vary depending upon the order in
+ * which values are recorded.
+ *
+ * This method may be implemented using compensated summation or
+ * other technique to reduce the error bound in the numerical sum
+ * used to compute the average.
+ *
+ * Values sorted by increasing absolute magnitude tend to yield
+ * more accurate results.
*
* @return the arithmetic mean of values, or zero if none
*/
--- old/src/share/classes/java/util/stream/DoubleStream.java 2013-10-09
16:37:23.0 -0700
+++ new/src/share/classes/java/util/stream/DoubleStream.java 2013-10-09
16:37:23.0 -0700
@@ -502,22 +502,42 @@
BiConsumerR, R combiner);
/**
- * Returns the sum of elements in this stream. The sum returned can vary
- * depending upon the order in which elements are encountered. This is
due
- * to accumulated rounding error in addition of values of differing
- * magnitudes. Elements sorted by increasing absolute magnitude tend to
- * yield more accurate results. If any stream element is a {@code NaN}
or
- * the sum is at any point a {@code NaN} then the sum will be {@code
NaN}.
- * This is a special case of a
- * a href=package-summary.html#Reductionreduction/a and is
+ * Returns the sum of elements in this stream.
+ *
+ * Summation is a special case of a a
+ *
Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown
On Oct 9, 2013, at 11:16 AM, Brian Burkhalter wrote:
For Windows I added a similar NET_ ThrowUnknownHostExceptionWithGaiError and
modified Inet{4,6} to mimic the Unix case. Note that the Windows code has not
yet been compiled pending comments.
Since compiled successfully on Windows but holding off on JPRT until there are
comments.
Thanks,
Brian
hg: jdk8/tl/jdk: 7189139: BigInteger's staticRandom field can be a source of bottlenecks.
Changeset: 673f8045311e Author:bpb Date: 2013-10-09 17:22 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/673f8045311e 7189139: BigInteger's staticRandom field can be a source of bottlenecks. Summary: Use ThreadLocalRandom instead of SecureRandom. Reviewed-by: shade, psandoz Contributed-by: Brian Burkhalter brian.burkhal...@oracle.com ! src/share/classes/java/math/BigInteger.java
Re: JDK 8 RFC 7189139 version 2: BigInteger's staticRandom field can be a source of bottlenecks
On Oct 9, 2013, at 5:33 AM, Paul Sandoz wrote: Nice! Perhaps as a separate bug you could place that code in the JDK test area as a non-jtreg test? Please see: https://bugs.openjdk.java.net/browse/JDK-8026236 The file of primes would need to be hosted elsewhere than the repository given its size. Tip: you can use Files.lines().map(BigInteger::new) but we don't current have a limitWhen operation so need to limit to N primes and not the content. And... a bonus we can now go parallel and since TLR is used there is no longer the contention point as with SR, which means we go faster! I have added a modified version of the test code (see end of email) if you want to play. If you place this test in the JDK test area i strongly recommend using streams, it's a nice use-case we can point people to. Here is the difference for the first 100 primes (with your patch applied) for parallel and sequential execution (which also includes the fixed cost of loading the primes from disk): $ time java PrimeTest primes-50M.txt 100 100 true By way of comparison here are some other results of the same test --- SecureRandom, parallel = false --- real 3m36.360s --- SecureRandom, parallel = true --- real 3m11.726s --- ThreadLocalRandom, parallel = false --- real2m34.260s --- ThreadLocalRandom, parallel = true --- real 1m39.173s These are of course not real benchmarks, especially as I was running on my dev system, but they are encouraging. 16s vs. 55s :-) The most salient point here is clearly that my machine is *much* slower. ;-) An updated webrev which differs only in having a correct Hg header is here: http://cr.openjdk.java.net/~bpb/7189139/webrev.2/ If this looks good to go, would a Reviewer please issue an approval? +1 Done: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/673f8045311e. Thanks, Brian
Re: JDK 8 code review request forJDK-8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation
On 10/09/2013 04:47 PM, Mike Duigou wrote: Big improvement. This looks like the right direction. DoubleStream.average() needs the same treatment for completeness. Thanks Mike. Next iteration incorporating this feedback (and some other off-list feedback): http://cr.openjdk.java.net/~darcy/8024354.2 -Joe
Re: JDK 8 code review request forJDK-8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation
For consistency I would move the this is a terminal operation paragraph to just before the @apiNote. I would suggest after the @apiNote but there's no mechanism for ending a javadoc tag other than starting another tag and we don't want the terminal text in the apiNote. Other than that it looks correct. Mike On Oct 9 2013, at 17:57 , Joe Darcy wrote: On 10/09/2013 04:47 PM, Mike Duigou wrote: Big improvement. This looks like the right direction. DoubleStream.average() needs the same treatment for completeness. Thanks Mike. Next iteration incorporating this feedback (and some other off-list feedback): http://cr.openjdk.java.net/~darcy/8024354.2 -Joe
Re: JDK 8 code review request forJDK-8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation
I'll pushed with the terminal op reordering you've suggested; thanks for the reviews, -Joe On 10/9/2013 6:17 PM, Mike Duigou wrote: For consistency I would move the this is a terminal operation paragraph to just before the @apiNote. I would suggest after the @apiNote but there's no mechanism for ending a javadoc tag other than starting another tag and we don't want the terminal text in the apiNote. Other than that it looks correct. Mike On Oct 9 2013, at 17:57 , Joe Darcy wrote: On 10/09/2013 04:47 PM, Mike Duigou wrote: Big improvement. This looks like the right direction. DoubleStream.average() needs the same treatment for completeness. Thanks Mike. Next iteration incorporating this feedback (and some other off-list feedback): http://cr.openjdk.java.net/~darcy/8024354.2 -Joe
hg: jdk8/tl/jdk: 8024709: TreeMap.DescendingKeyIterator 'remove' confuses iterator position
Changeset: eab3c09745b6 Author:bchristi Date: 2013-10-09 12:13 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/eab3c09745b6 8024709: TreeMap.DescendingKeyIterator 'remove' confuses iterator position Summary: Override remove() method in DescendingKeyIterator Reviewed-by: alanb, mduigou, psandoz ! src/share/classes/java/util/TreeMap.java ! test/java/util/Collection/MOAT.java
hg: jdk8/tl/jdk: 8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation
Changeset: c13309f658e1 Author:darcy Date: 2013-10-09 18:31 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c13309f658e1 8024354: Explicitly permit DoubleStream.sum()/average() implementations to use higher precision summation Reviewed-by: mduigou, briangoetz ! src/share/classes/java/util/DoubleSummaryStatistics.java ! src/share/classes/java/util/stream/DoubleStream.java
Re: RFR: 7044282: (reflect) Class.forName and Array.newInstance are inconsistent regarding multidimensional arrays
Hi Joel,
The code changes look fine, but I'd like to see some refactoring to the
tests. In particular, please put the logic in
81 try {
82 Class? c256 = Class.forName(name256);
83 error++;
84 System.err.println(ERROR: could create + c256);
85 } catch (ClassNotFoundException e) {
86 ;// ok
87 }
into a method that can be called like failingForName(name, clazz) (or
whatever is appropriate).
Thanks,
-Joe
On 10/9/2013 11:33 AM, Joel Borggren-Franck wrote:
Hi
Please review this spec update and test for getting array classes and
instances of more dimensions than the class file can express or the VM
can handle.
Array.newInstance have a test for arrays of more dimensions than 255,
this patch adds a test for Class.forName as well.
Also the javadoc for Array.newInstance are clarified.
Bug: https://bugs.openjdk.java.net/browse/JDK-7044282
Webrev: http://cr.openjdk.java.net/~jfranck/7044282/webrev.00/
cheers
/Joel
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
Hi Brian,
In the text:
* @exception NullPointerException if {@code urls} or any of its elements
* is {@code null}
I'm unsure if it should be is null or are null given the initial
subject is singular and latter subject is plural. Existing similar
phrases seem to be split roughly 50-50.
Nit: In the test there are a few places where you have t on a line by
itself:
65t);
but it can go on the previous line and not exceed the length of other
lines nearby. Also + should be +
Thanks,
David
On 10/10/2013 6:01 AM, Brian Burkhalter wrote:
On Oct 9, 2013, at 3:06 AM, Michael McMahon wrote:
If I read the code correctly then the long standing behavior of
URLClassLoader.getPermissions(null) was to throw a NPE, now it is changed to
return the empty collection in order to match the super type
(SecureClassLoader). I can't think of anything that would break so this is
probably okay, just maybe a bit surprising.
I'm still not sure about that change. Its effect in sub-classes will be to
continue on in the sub-class implementation
of getPermissions() with a null codesource parameter, where currently NPE would
be thrown.
Well, upon re-examination I actually agree with you, especially considering the
class hierarchy:
AppletClassLoader :: URLClassLoader :: SecureClassLoader :: ClassLoader
I have posted an updated webrev here:
http://cr.openjdk.java.net/~bpb/7179567/webrev.4/
The differences with respect to the previous webrev [1] are:
1) the javadoc change of SecureClassLoader has been reverted;
2) these statements
665 if (codesource == null) {
666 return perms;
667 }
668
in URLClassLoader.getPermissions() have been reverted;
3) the javadoc of getPermissions() in URLClassLoader and AppletClassLoader has been
updated to add @exception NullPointerException if {@code codesource} is {@code
null}.
The CCC request to be associated with this change would indicate that @exception
NullPointerException has been added to the javadoc of:
A) all public URLClassLoader constructors;
B) URLClassLoader.findClass();
C) URLClassLoader.getPermissions();
D) both URLClassLoader.newInstance() methods;
E) AppletClassLoader.getPermissions().
The only actual *code* changes are in URLClassLoader.
Thanks,
Brian
[1] http://cr.openjdk.java.net/~bpb/7179567/webrev.3/
Re: RFR: 8024688: j.u.Map.merge doesn't work as specified if contains key:null pair
On Oct 8 2013, at 01:27 , Paul Sandoz wrote: Hi Mike, I am probably going over old ground here... Given that there is ConcurrentMap is there any point in having the defaults on Map detect concurrent modification and barf, or retry, or neither of the previous two e.g. putIfAbsent, computeIfPresent and replace respectively i.e. there seems to be inconsistent behaviour here. Would it not be better to separate concerns of serial and concurrent access in the defaults of Map and ConcurrentMap? I have created a prototype renovated Map and ConcurrentMap to provide (mostly) separate implementations. http://cr.openjdk.java.net/~mduigou/JDK-8024688.2/0/webrev/ This passes all of the existing regression tests and I believe is performance neutral. The implementations are now more relatively correct for both flavours. ie. Map defaults don't retry and throw CME when they detect that something changed unexpectedly. The ConcurrentMap defaults were mostly just moving over the prior Map defaults but I also removed a few cases of null value handling that were no longer needed. I've wanted to explore this for a while (since June) but haven't previously had a chance. I'm somewhat convinced that this is the right direction to be going but I am sure further refinement is possible (particularly in regards to the spec). The defaults on Map could state they are not suitable for maps that can be concurrently modified, if that is required extend from ConcurrentMap. If function values passed to methods modify the Map then undefined behaviour will result. (I am also wondering if there are currently edge cases e.g. if a function value modifies the source then the re-try loop will never terminate.) I realize that is separate from the null behaviour, but perhaps this separate will help clarify null behaviour? I realized that some of the statements about implementations must document how they handle nulls with this method were no longer relevant. The null handling behaviour was entirely covered by the class documentation and the method specification. This is mostly a consequence of having added or is mapped to null value in couple of cases later on in the process. Thanks for pushing on this point (and David Holmes for earlier suggestion that this might be important) Mike Paul. On Oct 4, 2013, at 5:35 AM, Mike Duigou mike.dui...@oracle.com wrote: Hello all; This is a changeset which improves the consistency of several Map.merge implementations for handling of null values. The existing unit tests hadn't considered several cases where the result of the remapper was not the same as the value. I've restructured the merge tests to be more thorough and systematic this revealed a couple of problems. http://cr.openjdk.java.net/~mduigou/JDK-8024688/0/webrev/ Like several of the previous patches, this one introduces an alternative default for ConcurrentMap to work around issues involving null values where the handling in the general Map default would be incorrect. Mike
Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE
On Oct 9, 2013, at 8:32 PM, David Holmes david.hol...@oracle.com wrote: I'm unsure if it should be is null or are null given the initial subject is singular and latter subject is plural. The English major in me surfaces briefly to note that any can be either singular or plural: Pick a card, any card (singular) vs. any takers? (plural). Brian's works well enough as a singular. Etymologically, any (like an) is a cognate of the word one. — John
Re: 7011859: java/util/concurrent/Semaphore/RacingReleases.java failing
On 9/10/2013 9:55 PM, Alan Bateman wrote: This test has been failing (very intermittently) for some time. It was assumed to be a HotSpot issue on SPARC but more recently it has been duplicated on Linux x64 too. David Simms has notes in the bug [1] from this instrumentation and analysis, and Doug has pushed a fix to his CVS that fixes the issue (although they may still be an underlying issue that still needs to be hunted down). Doug's fix updates AQS and AQLS to re-read the head and avoid the race. We need to bring this into jdk8/tl, the webrev with the patch is here: http://cr.openjdk.java.net/~alanb/7011859/webrev/ Reviewed! Thanks Alan (and Doug and David S.) David H. -Alan [1] https://bugs.openjdk.java.net/browse/JDK-7011859
