Re: Request for Review : CR#8004015 : [2nd pass] Add interface extends and defaults for basic functional interfaces
I am still wondering if we need some sort of javadoc tag for default implementations so that it will stand out better and allow us to be consistent with how we specify this across Java SE and other APIs that leverage default methods. Has any thought been given to this? Best Lance On Dec 5, 2012, at 12:47 AM, Mike Duigou wrote: Hello all; I have updated the proposed patch. The changes primarily add class and method documentation regarding handling of null for the primitive specializations. http://cr.openjdk.java.net/~mduigou/8004015/1/webrev/ http://cr.openjdk.java.net/~mduigou/8004015/1/specdiff/java/util/function/package-summary.html I've also reformatted the source for the default methods. Mike On Nov 26 2012, at 18:12 , Mike Duigou wrote: Hello all; In the original patch which added the basic lambda functional interfaces, CR#8001634 [1], none of the interfaces extended other interfaces. The reason was primarily that the javac compiler did not, at the time that 8001634 was proposed, support extension methods. The compiler now supports adding of method defaults so this patch improves the functional interfaces by filing in the inheritance hierarchy. Adding the parent interfaces and default methods allows each functional interface to be used in more places. It is especially important for the functional interfaces which support primitive types, IntSupplier, IntFunction, IntUnaryOperator, IntBinaryOperator, etc. We expect that eventually standard implementations of these interfaces will be provided for functions like max, min, sum, etc. By extending the reference oriented functional interfaces such as Function, the primitive implementations can be used with the boxed primitive types along with the primitive types for which they are defined. The patch to add parent interfaces and default methods can be found here: http://cr.openjdk.java.net/~mduigou/8004015/0/webrev/ http://cr.openjdk.java.net/~mduigou/8004015/0/specdiff/java/util/function/package-summary.html Mike [1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c2e80176a697 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Proposal: Fully Concurrent ClassLoading
Java 7 introduced support for parallel classloading by adding to each class loader a ConcurrentHashMap, referenced through a new field, parallelLockMap. This contains a mapping from class names to Objects to use as a classloading lock for that class name. This scheme has a number of inefficiencies. To address this we propose for Java 8 the notion of a fully concurrent classloader ... This is a fairly simple proposal that I've written up as a blog entry: https://blogs.oracle.com/dholmes/entry/parallel_classloading_revisited_fully_concurrent Please discuss this proposal here. Thanks, David Holmes
Re: CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)
On 12/5/12 3:45 PM, Alan Bateman wrote:
On 04/12/2012 15:55, Daniel Fuchs wrote:
So that would be the wording:
* Uses the service-provider loading facilities, defined by the
* {@link java.util.ServiceLoader} class, to attempt to locate
* and load an implementation of the service. If there are
* providers other than the implementation specific default
* located, then the first provider that is
* not the default is instantiated and returned; Otherwise
* the default implementation is returned if it is on the
* classpath or installed as an extension.
This however suggests that in JDK 8 the default implementation
*needs* to be put on the classpath or installed as an extension.
But is that right?
Wouldn't that be required only when running with a profile in
which the default implementation is not present by default?
Maybe we need to find yet a better wording.
-- daniel
It might be better to just leave it as Otherwise the default
implementation is returned.
Or maybe:
Otherwise, the default implementation, if present, is returned ?
I think the original intent was to make provision for the case where
the implementation module would not be installed
-- daniel
-Alan
Re: CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)
On 05/12/2012 14:56, Daniel Fuchs wrote: Or maybe: Otherwise, the default implementation, if present, is returned ? I think the original intent was to make provision for the case where the implementation module would not be installed That is longer term intent, meaning when we go do modules. So your proposal seems good to me. -Alan
Re: 8003562: Provide a command-line tool to find static dependencies
On 04/12/2012 20:15, Mandy Chung wrote: Alan - thanks for the feedback. I have made several changes to the jdeps tool. Here is the new webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8003562/langtools-webrev.01/ I'll send out a formal code review request once I add the new unit tests. I like the name of the tool, and I think the new proposed options looks much better. The only option that I suspect might confuse people is -d, I'm not sure that this is really needed. A minor observation but I wonder if -v/--verbose should be something else. The sub-options are good, it's just that -v is really is selecting the level that the dependencies are printed (and -v:summary is less verbose than the default). Otherwise I think this will be really useful to have. -Alan,
Re: CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)
Hi, Please find below a revised version of the changes for the javax.xml.parsers package. It hopefully incorporates all comments I have received so far. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.01/ * better wording/formating for Javadoc changes * using for( : ) syntax in findServiceProvider * improved // comments explaining why the additional layer of RuntimeException is needed when wrapping ServiceConfigurationError. best regards, -- daniel On 12/4/12 2:54 PM, Alan Bateman wrote: On 03/12/2012 19:04, Daniel Fuchs wrote: Hi, This is a first webrev in a series that addresses a change intended for JDK 8: 7169894: JAXP Plugability Layer: using service loader I've been building up on Joe's work and Paul Alan comments from an earlier discussion thread [1] This here addresses changes in the javax.xml.parsers package for the SAXParserFactory and DocumentBuilderFactory - and more specifically their no-argument newInstance() method. This change replaces the custom code that was reading the META-INF/services/ resources by a simple call to ServiceLoader. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.00/ Thank you very much for taking this one on. I think your approach to take javax.xml.parsers on its own is good as the previous review rounds got really stuck in the mire due to the number of factories, code duplication, and subtle specification and implementation differences. In the class descriptions it suggests that the default implementation may be installed as a module but we aren't there yet so I'm not sure about using the term module. I think it should be okay to say installed as an extension as ServiceLoader uses this term. The class description also suggests that a SCE will be wrapped as a FactoryConfigurationException but in FactoryFinder I see that it actual wraps a RuntimeException. I think you can just use the no-arg constructor then then use initCause to set the cause to the SCE. Also the statement If a misconfigured provider is encountered and SCE is thrown can probably be changed to If SCE is thrown as it can be thrown for other reasons too. Minor nit is that the updates to DocumentBuilderFactory's class description requires a really wide screen compared to the rest of the javadoc. Minor implementation suggestion for findServiceProvider is that you can use for-each to iterate through the providers. Otherwise I think you've captured all the points of feedback from the original review. Finally one suggestion is to make keep notes on the before after behavior, this is something that will be important for release and compatibility notes. -Alan.
Re: RFR: 8004317 TestLibrary.getUnusedRandomPort() fails intermittently, but exception not reported
Thanks for the suggestions, Stuart. BTW printStackTrace() prints to standard error by default -- that's why I don't explicitly have it in there. Cheers, Jim On 12/04/2012 07:06 PM, Stuart Marks wrote: Hi Jim, (Looks like you're cleaning up warnings along the way. I guess that's OK.) Before printing the stack trace, there should be a message to stderr indicating where this stack trace is coming from. For example, getUnusedRandomPort: caught exception. The stack trace should be printed to stderr as well, using something like ex.printStackTrace(System.err). I think narrowing the catch to IOException is good, since that's the only exception case we really want to retry. I don't think it makes sense to mention IllegalArgumentException or SecurityException specifically in the comments though. Any exception other than IOException should fail-fast. A message should also be printed if we decide to retry because the port is one of the reserved ports. This might provide an important clue to solving the puzzle. (My hypothesis is that this routine fails relatively silently when, on its last retry, it successfully opens a reserved port. In this case ex will be null and we get the RuntimeException with no further explanation.) It would also be helpful to print numTries each time around the loop so that we can see if it really is retrying that many times. Regarding netstat, I think it's a good idea, but I'd suggest we work on it separately from this change. Instead, suppose we add a shell script test that's named so that it runs at the end of the jdk_rmi test target. This could just run netstat -a (or whatever) unconditionally. In fact, I could do that without even pushing any changes to the source code s'marks On 12/4/12 1:42 PM, Jim Gish wrote: OK -- how about a push then for now and with luck we might get some useful output in a day or two. Jim On 12/04/2012 04:22 PM, Alan Bateman wrote: On 04/12/2012 21:10, Jim Gish wrote: : P.S. working on adding nestat -a output per Alan's suggestion. BTW: I didn't mean you have to run with my comment, it's just that I assume the issue that the Windows is out of dynamic ports. There is a registry setting to change the range, and I think there is a netsh command to adjust it too. If we can somehow capture the netstat output then it may confirm this. -Alan. -- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.g...@oracle.com
Re: Request for Review : CR#8004015 : [2nd pass] Add interface extends and defaults for basic functional interfaces
On Dec 4 2012, at 22:10 , David Holmes wrote:
Hi Mike,
In multiple places:
+ * p/xxx ...
Should that be p tag? Is it actually needed? (my javadoc is a bit rusty).
Many of these were added/changed by NetBeans styler. I then added additional
instances. I have converted all of the p/ - p.
I have also filed a bug against NetBans styler:
http://netbeans.org/bugzilla/show_bug.cgi?id=223342
Aside: I don't realise you could {@inheritDoc) as a simple text insertion
mechanism.
I only learned of this in the last six months myself. :-)
Just to be clear, the null-handling statements are intended to be normative
and apply to anyone who might provide an implementation of theses classes -
right?
Correct. I would prefer that they were not but it seems unavoidable.
Mike
Thanks,
David
On 5/12/2012 3:47 PM, Mike Duigou wrote:
Hello all;
I have updated the proposed patch. The changes primarily add class and
method documentation regarding handling of null for the primitive
specializations.
http://cr.openjdk.java.net/~mduigou/8004015/1/webrev/
http://cr.openjdk.java.net/~mduigou/8004015/1/specdiff/java/util/function/package-summary.html
I've also reformatted the source for the default methods.
Mike
On Nov 26 2012, at 18:12 , Mike Duigou wrote:
Hello all;
In the original patch which added the basic lambda functional interfaces,
CR#8001634 [1], none of the interfaces extended other interfaces. The
reason was primarily that the javac compiler did not, at the time that
8001634 was proposed, support extension methods. The compiler now supports
adding of method defaults so this patch improves the functional interfaces
by filing in the inheritance hierarchy.
Adding the parent interfaces and default methods allows each functional
interface to be used in more places. It is especially important for the
functional interfaces which support primitive types, IntSupplier,
IntFunction, IntUnaryOperator, IntBinaryOperator, etc. We expect that
eventually standard implementations of these interfaces will be provided
for functions like max, min, sum, etc. By extending the reference oriented
functional interfaces such as Function, the primitive implementations can
be used with the boxed primitive types along with the primitive types for
which they are defined.
The patch to add parent interfaces and default methods can be found here:
http://cr.openjdk.java.net/~mduigou/8004015/0/webrev/
http://cr.openjdk.java.net/~mduigou/8004015/0/specdiff/java/util/function/package-summary.html
Mike
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c2e80176a697
Re: CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)
Hi Daniel, Looks good to me! -Joe On 12/5/2012 8:17 AM, Daniel Fuchs wrote: Hi, Please find below a revised version of the changes for the javax.xml.parsers package. It hopefully incorporates all comments I have received so far. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.01/ * better wording/formating for Javadoc changes * using for( : ) syntax in findServiceProvider * improved // comments explaining why the additional layer of RuntimeException is needed when wrapping ServiceConfigurationError. best regards, -- daniel On 12/4/12 2:54 PM, Alan Bateman wrote: On 03/12/2012 19:04, Daniel Fuchs wrote: Hi, This is a first webrev in a series that addresses a change intended for JDK 8: 7169894: JAXP Plugability Layer: using service loader I've been building up on Joe's work and Paul Alan comments from an earlier discussion thread [1] This here addresses changes in the javax.xml.parsers package for the SAXParserFactory and DocumentBuilderFactory - and more specifically their no-argument newInstance() method. This change replaces the custom code that was reading the META-INF/services/ resources by a simple call to ServiceLoader. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.00/ Thank you very much for taking this one on. I think your approach to take javax.xml.parsers on its own is good as the previous review rounds got really stuck in the mire due to the number of factories, code duplication, and subtle specification and implementation differences. In the class descriptions it suggests that the default implementation may be installed as a module but we aren't there yet so I'm not sure about using the term module. I think it should be okay to say installed as an extension as ServiceLoader uses this term. The class description also suggests that a SCE will be wrapped as a FactoryConfigurationException but in FactoryFinder I see that it actual wraps a RuntimeException. I think you can just use the no-arg constructor then then use initCause to set the cause to the SCE. Also the statement If a misconfigured provider is encountered and SCE is thrown can probably be changed to If SCE is thrown as it can be thrown for other reasons too. Minor nit is that the updates to DocumentBuilderFactory's class description requires a really wide screen compared to the rest of the javadoc. Minor implementation suggestion for findServiceProvider is that you can use for-each to iterate through the providers. Otherwise I think you've captured all the points of feedback from the original review. Finally one suggestion is to make keep notes on the before after behavior, this is something that will be important for release and compatibility notes. -Alan.
8004371: (props) Properties.loadFromXML needs small footprint XML parser as fallback when JAXP is not present
A while back [1], I brought up the issue of the Properties storeToXML/loadFromXML methods being problematic for our efforts to modularize the JDK and also the Compact Profiles effort. At the time I mentioned that we were thinking of including a small footprint XML parser that we could use in the base module (when JAXP is not present) and also use in the compact1 profile. The preparatory work to create a JDK-internal provider mechanism and the spec work to only require an implementation support UTF-8 and UTF-16 is already in jdk8, now it's time for the next step. Joe Wang has taken the JSR-280 RI, which includes some code from SAX, and stripped it down so that it's reasonably small (less than 50k in a compressed rt.jar). We've hooked it up to Properties so that it works as a fallback when there isn't an XmlPropertiesProvider present. There's still a good bit of clean-up required and there's probably more that can be pulled out but it's at the point where we can start early review. To that end, the webrev with everything is here: http://cr.openjdk.java.net/~alanb/8004371/webrev/ For tests then I've changed the LoadAndStoreXML test that I added recently to exercise the implementation. Otherwise, the implementation will only be used when testing the smallest profile (compact1, brewing in the jdk8/profiles forest) or when it gets pulled into the downstream jigsaw forest. Joe has some additional tests and we need to compare these with existing tests to see if it's worth converting them. One issue that I'm still mulling over, as least for the profiles effort, is whether to only include the fallback provider in the smallest profile as it won't be used otherwise. If the eventual size isn't too significant then it might not be worth it of course. -Alan. [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-October/011648.html
RFR: 8003246: Add Supplier to ThreadLocal
This patch adds a constructor to ThreadLocal to supply initial values using the new Supplier functional interface. Please review. This work was done by Jim Gish. http://cr.openjdk.java.net/~akhil/8003246.0/webrev/ http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003246 Thanks
Re: RFR: 8004317 TestLibrary.getUnusedRandomPort() fails intermittently, but exception not reported
On 12/5/12 8:41 AM, Jim Gish wrote: BTW printStackTrace() prints to standard error by default -- that's why I don't explicitly have it in there. Oh yes, so it does. Sorry, I was confused. s'marks
Re: RFR: 8003246: Add Supplier to ThreadLocal
Looks good to me. Mike On Dec 5 2012, at 11:39 , Akhil Arora wrote: This patch adds a constructor to ThreadLocal to supply initial values using the new Supplier functional interface. Please review. This work was done by Jim Gish. http://cr.openjdk.java.net/~akhil/8003246.0/webrev/ http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003246 Thanks
Re: RFR: 8003246: Add Supplier to ThreadLocal
On 12/05/12 14:39, Akhil Arora wrote: This patch adds a constructor to ThreadLocal to supply initial values using the new Supplier functional interface. Please review. This work was done by Jim Gish. http://cr.openjdk.java.net/~akhil/8003246.0/webrev/ http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003246 Thanks I see that EVERY ThreadLocal now has an extra field in case it is used with a Supplier. I expect that those web frameworks that create thousands of ThreadLocals (not just instances of Threadlocals) will see a measurable space increase. has anyone measure the impact? Did anyone consider instead defining a SuppliedThreadLocal subclass (or some better name) to isolate the impact? -Doug
Re: RFR: 8003246: Add Supplier to ThreadLocal
On 12/05/2012 08:51 PM, Doug Lea wrote: On 12/05/12 14:39, Akhil Arora wrote: This patch adds a constructor to ThreadLocal to supply initial values using the new Supplier functional interface. Please review. This work was done by Jim Gish. http://cr.openjdk.java.net/~akhil/8003246.0/webrev/ http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003246 Thanks I see that EVERY ThreadLocal now has an extra field in case it is used with a Supplier. I expect that those web frameworks that create thousands of ThreadLocals (not just instances of Threadlocals) will see a measurable space increase. has anyone measure the impact? Did anyone consider instead defining a SuppliedThreadLocal subclass (or some better name) to isolate the impact? The class doesn't even to have a name, one can add a static factory method in ThreadLocal and use an anonymous class to implement it. The other problem is that SupplierT should be replaced by Supplier? extends T everywhere in the code. -Doug Rémi
Re: RFR: 8004317 TestLibrary.getUnusedRandomPort() fails intermittently, but exception not reported
Here's a new version for your consideration :-) http://cr.openjdk.java.net/~jgish/Bug8004317-TestLibrary-getUnusedRandomPort-Failure/ http://cr.openjdk.java.net/%7Ejgish/Bug8004317-TestLibrary-getUnusedRandomPort-Failure/ Thanks, Jim On 12/05/2012 02:45 PM, Stuart Marks wrote: On 12/5/12 8:41 AM, Jim Gish wrote: BTW printStackTrace() prints to standard error by default -- that's why I don't explicitly have it in there. Oh yes, so it does. Sorry, I was confused. s'marks -- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.g...@oracle.com
Re: Review Request: 8004201: add reducers to primitive type wrappers
Updated - http://cr.openjdk.java.net/~akhil/8004201.1/webrev/ - delegate to Math.min/max for int/long/float/double - rename Boolean.and/or/xor to logicalAnd/logicalOr/logicalXor - removed Character variants of min/max/sum On 12/02/2012 05:50 PM, David Holmes wrote: Hi Akhil, Is it really necessary/desirable to flag all of these as Suitable for conversion as a method reference to functional interfaces such as ... ? Not necessary, but it does provide a hint as to their intended use to a casual browser of these docs. This style: + * @param a a boolean argument. + * @param b another boolean argument. is at odds with the style used elsewhere for new Functional APIs, and with the style of other methods in these classes. Can we just use first operand and second operand for all of these? It is consistent with Math.min/max, which use the a/b style. Since these methods are not in one of the functional package, is'nt it better to stick to the local style? Character.sum does not make sense to me. Who adds together characters? I'm not even sure min and max are worth supporting for Character. Good point - removed these methods for Character. I disagree with other suggestions to use the Math functions for float/double. I think all these methods should use the underlying primitive operator regardless of type. Are you disagreeing only for float/double or for int/long also? Can you provide more information as to why you disagree? Thanks Thanks, David - On 1/12/2012 4:44 AM, Akhil Arora wrote: Hi Requesting review for some basic functionality related to lambdas - Add min, max, sum methods to the primitive wrapper classes - Byte, Short, Integer, Long, Float, Double and Character so as to be able to use them as reducers in lambda expressions. Add and, or, xor methods to Boolean. http://cr.openjdk.java.net/~akhil/8004201.0/webrev/ http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004201 Thanks
Re: Review Request: 8004201: add reducers to primitive type wrappers
Regarding Character methods; it's unorthodox but some people use them as fake unsigned shorts. Whether that's enough to justify adding sum/max/min, I don't know. Sent from my phone On Dec 5, 2012 4:28 PM, Akhil Arora akhil.ar...@oracle.com wrote: Updated - http://cr.openjdk.java.net/~**akhil/8004201.1/webrev/http://cr.openjdk.java.net/~akhil/8004201.1/webrev/ - delegate to Math.min/max for int/long/float/double - rename Boolean.and/or/xor to logicalAnd/logicalOr/**logicalXor - removed Character variants of min/max/sum On 12/02/2012 05:50 PM, David Holmes wrote: Hi Akhil, Is it really necessary/desirable to flag all of these as Suitable for conversion as a method reference to functional interfaces such as ... ? Not necessary, but it does provide a hint as to their intended use to a casual browser of these docs. This style: + * @param a a boolean argument. + * @param b another boolean argument. is at odds with the style used elsewhere for new Functional APIs, and with the style of other methods in these classes. Can we just use first operand and second operand for all of these? It is consistent with Math.min/max, which use the a/b style. Since these methods are not in one of the functional package, is'nt it better to stick to the local style? Character.sum does not make sense to me. Who adds together characters? I'm not even sure min and max are worth supporting for Character. Good point - removed these methods for Character. I disagree with other suggestions to use the Math functions for float/double. I think all these methods should use the underlying primitive operator regardless of type. Are you disagreeing only for float/double or for int/long also? Can you provide more information as to why you disagree? Thanks Thanks, David - On 1/12/2012 4:44 AM, Akhil Arora wrote: Hi Requesting review for some basic functionality related to lambdas - Add min, max, sum methods to the primitive wrapper classes - Byte, Short, Integer, Long, Float, Double and Character so as to be able to use them as reducers in lambda expressions. Add and, or, xor methods to Boolean. http://cr.openjdk.java.net/~**akhil/8004201.0/webrev/http://cr.openjdk.java.net/~akhil/8004201.0/webrev/ http://bugs.sun.com/**bugdatabase/view_bug.do?bug_**id=8004201http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004201 Thanks
Re: CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)
Hi Daniel, Looks good to me. One question - the last bullet in the search order covers the default implementation case: Platform default codeDocumentBuilderFactory/code instance. Would it make sense to merge the statement Otherwise, the default implementation, if present, is returned with the above statement? e.g. Default implementation of codeDocumentBuilderFactory/code if present. Mandy On 12/5/2012 8:17 AM, Daniel Fuchs wrote: Hi, Please find below a revised version of the changes for the javax.xml.parsers package. It hopefully incorporates all comments I have received so far. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.01/ * better wording/formating for Javadoc changes * using for( : ) syntax in findServiceProvider * improved // comments explaining why the additional layer of RuntimeException is needed when wrapping ServiceConfigurationError. best regards, -- daniel On 12/4/12 2:54 PM, Alan Bateman wrote: On 03/12/2012 19:04, Daniel Fuchs wrote: Hi, This is a first webrev in a series that addresses a change intended for JDK 8: 7169894: JAXP Plugability Layer: using service loader I've been building up on Joe's work and Paul Alan comments from an earlier discussion thread [1] This here addresses changes in the javax.xml.parsers package for the SAXParserFactory and DocumentBuilderFactory - and more specifically their no-argument newInstance() method. This change replaces the custom code that was reading the META-INF/services/ resources by a simple call to ServiceLoader. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.parsers/webrev.00/ Thank you very much for taking this one on. I think your approach to take javax.xml.parsers on its own is good as the previous review rounds got really stuck in the mire due to the number of factories, code duplication, and subtle specification and implementation differences. In the class descriptions it suggests that the default implementation may be installed as a module but we aren't there yet so I'm not sure about using the term module. I think it should be okay to say installed as an extension as ServiceLoader uses this term. The class description also suggests that a SCE will be wrapped as a FactoryConfigurationException but in FactoryFinder I see that it actual wraps a RuntimeException. I think you can just use the no-arg constructor then then use initCause to set the cause to the SCE. Also the statement If a misconfigured provider is encountered and SCE is thrown can probably be changed to If SCE is thrown as it can be thrown for other reasons too. Minor nit is that the updates to DocumentBuilderFactory's class description requires a really wide screen compared to the rest of the javadoc. Minor implementation suggestion for findServiceProvider is that you can use for-each to iterate through the providers. Otherwise I think you've captured all the points of feedback from the original review. Finally one suggestion is to make keep notes on the before after behavior, this is something that will be important for release and compatibility notes. -Alan.
Re: 8003562: Provide a command-line tool to find static dependencies
Yeah, this is a great tool. Does it also help to find the need to recompile a class if the source of a inlined referred static final constant has changed? -Ulf Am 28.11.2012 00:18, schrieb Mandy Chung: As part of prepare for modules [1], this RFE is to provide a command-line tool in JDK8 so that developers can understand the static dependencies of their applications and libraries.As part of Project Jigsaw, a useful class analyzer [2] was developed that makes it very easy to identify the dependencies based on the classfile library thathas also been enhanced to support dependency analysis [3]. Inspired by the sample tool that Jon Gibbons developed, we propose this new command-line name as jdeps. $ ./bin/jdeps -h Usage: jdeps options files where possible options include: -version Version information -classpath pathSpecify where to find class files -v --verbosePrint class-level dependencies -r --reverseInvert the dependencies in the output -p Restrict analysis to classes in this package (may be given multiple times) -e --regex Restrict analysis to packages matching pattern (-p and -e are exclusive) -P --profileShow profile or the file containing a package -d --depth Specify the depth of the transitive dependency analysis Default depth is 1. Set depth to 0 to traverse all dependencies. -all Show all classes and members with no breakdown The jdeps tool shows package-level dependencies of the input files that can be .class files, a directory, or a JAR file. Specify the depth for the transitive dependency analysis; otherwise, it only analyzes the input files. jdeps -P option will show where the class/package comes from. For Java SE API, it will show the Profile name (I implement a workaround for now until the profile work is in jdk8). For JDK internal APIs, they will not be exported in modular world. jdeps will indicate any usage of the JDK internal APIs in the output to help developers prepare for transitioning to modules. See below for a few sample output. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8003562/ The implementation classes for jdeps are in the langtools repo along with the com.sun.tools.classfile library. I'm working on adding more unit tests. I'd like to get this webrev out to begin the discussion and get review feedback. Thanks Mandy [1] http://openjdk.java.net/jeps/162 [2] http://hg.openjdk.java.net/jigsaw/jigsaw/jdk/raw-file/543b0d24a920/make/tools/classanalyzer/classanalyzer.html [3] http://bugs.sun.com/view_bug.do?bug_id=6907575 Sample Output $./bin/jdeps demo/jfc/Notepad/Notepad.jar unnamed (demo/jfc/Notepad/Notepad.jar) - java.awt - java.awt.event - java.beans - java.io - java.lang - java.net - java.util - java.util.logging - javax.swing - javax.swing.border - javax.swing.event - javax.swing.text - javax.swing.tree - javax.swing.undo $ ./bin/jdeps -P demo/jfc/Notepad/Notepad.jar unnamed (demo/jfc/Notepad/Notepad.jar) - java.awt compact4 - java.awt.event compact4 - java.beans compact4 - java.io compact1 - java.langcompact1 - java.net compact1 - java.utilcompact1 - java.util.loggingcompact1 - javax.swing compact4 - javax.swing.border compact4 - javax.swing.eventcompact4 - javax.swing.text compact4 - javax.swing.tree compact4 - javax.swing.undo compact4 $ ./bin/jdeps -d 10 demo/jfc/Notepad/Notepad.jar unnamed (demo/jfc/Notepad/Notepad.jar) - java.awt - java.awt.event - java.beans - java.io - java.lang - java.net - java.util - java.util.logging - javax.swing - javax.swing.border - javax.swing.event - javax.swing.text - javax.swing.tree - javax.swing.undo java.security (/export/mchung/ws/jdeps-repo/build/macosx-x86_64/j2sdk-image/jre/lib/rt.jar) - javax.crypto javax.crypto (/export/mchung/ws/jdeps-repo/build/macosx-x86_64/j2sdk-image/jre/lib/jce.jar) - java.io - java.lang - java.lang.reflect - java.net - java.nio - java.security - java.security.cert - java.security.spec - java.util - java.util.concurrent - java.util.jar - java.util.regex - java.util.zip - sun.security.jca JDK internal API - sun.security.utilJDK internal API javax.crypto.spec
Review request: 8004042 : Arrrghs.java test failed on windows with access error.
A fix for intermittent test failures causing grief on Windows, particularly Windows 7 and 2008 systems: http://cr.openjdk.java.net/~ddehaven/8004042/webrev.1/ The underlying cause is as of yet unknown, but I was able to create an environment that caused similar failures by running watch -n 0.2 cat JTwork/scratch/atest.bat in another shell then running jtreg. Without the patch it would fail very regularly (especially when isolated and looped 1,000 times), with the patch it always passes. -DrD-
Re: Review Request: 8004201: add reducers to primitive type wrappers
Akhil,
In javadoc like this
298 * as {@code BinaryOperatorlt;Booleangt;}.
you don't have to use lt; and the like inside {@code}; please change to
298 * as {@code BinaryOperatorBoolean}.
As a general comment, if the operations for primitive type Foo are put
into java.lang.Foo, then the type of the operation needs to be
explicitly represented in the expression calling the method (unless
static imports are used, etc.). Therefore, I suggest putting these sort
of static methods all into one class to allow overloading to do its
thing (java.lang.Operators?). Also, for methods like sum, I think it is
worth considering returning a larger type than the operands to avoid
problems from integer or floating-point overflow. For example, sum on
byte and short would return int, sun on int would return long, etc.
As an aside, to get a numerically robust result, the summation logic
over a set of doubles needs to be more than just a set of raw double
additions, but that can be tackled later.
Cheers,
-Joe
On 12/5/2012 1:27 PM, Akhil Arora wrote:
Updated - http://cr.openjdk.java.net/~akhil/8004201.1/webrev/
- delegate to Math.min/max for int/long/float/double
- rename Boolean.and/or/xor to logicalAnd/logicalOr/logicalXor
- removed Character variants of min/max/sum
On 12/02/2012 05:50 PM, David Holmes wrote:
Hi Akhil,
Is it really necessary/desirable to flag all of these as Suitable for
conversion as a method reference to functional interfaces such as ... ?
Not necessary, but it does provide a hint as to their intended use to a
casual browser of these docs.
This style:
+ * @param a a boolean argument.
+ * @param b another boolean argument.
is at odds with the style used elsewhere for new Functional APIs, and
with the style of other methods in these classes. Can we just use first
operand and second operand for all of these?
It is consistent with Math.min/max, which use the a/b style. Since these
methods are not in one of the functional package, is'nt it better to
stick to the local style?
Character.sum does not make sense to me. Who adds together characters?
I'm not even sure min and max are worth supporting for Character.
Good point - removed these methods for Character.
I disagree with other suggestions to use the Math functions for
float/double. I think all these methods should use the underlying
primitive operator regardless of type.
Are you disagreeing only for float/double or for int/long also? Can you
provide more information as to why you disagree?
Thanks
Thanks,
David
-
On 1/12/2012 4:44 AM, Akhil Arora wrote:
Hi
Requesting review for some basic functionality related to lambdas -
Add min, max, sum methods to the primitive wrapper classes - Byte,
Short, Integer, Long, Float, Double and Character so as to be able to
use them as reducers in lambda expressions. Add and, or, xor methods to
Boolean.
http://cr.openjdk.java.net/~akhil/8004201.0/webrev/
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004201
Thanks
Re: RFR: 8004317 TestLibrary.getUnusedRandomPort() fails intermittently, but exception not reported
OK, looks better, more explicit so that we can find out why this is failing. There's still a subtle issue in the reporting though. Consider if on attempt N the ServerSocket call gets a valid port but it's one of the reserved ports. Then, unusedRandomPort will be = 0 and isReservedPort() will be true, so we'll get the INFO message. Now on attempt N+1 suppose ServerSocket throws an exception. We'll get the exception stack trace, but then unusedRandomPort will still have its previous value, and we'll get the INFO message again, but spuriously this time. I hate to ask you to update this again, but as it stands I think the output will be quite confusing. I think setting unusedRandomPort back to -1 at the top of the loop should fix it. You need me to push this for you? I can drop in this change before I push, if you're OK with me doing this. s'marks On 12/5/12 12:51 PM, Jim Gish wrote: Here's a new version for your consideration :-) http://cr.openjdk.java.net/~jgish/Bug8004317-TestLibrary-getUnusedRandomPort-Failure/ http://cr.openjdk.java.net/%7Ejgish/Bug8004317-TestLibrary-getUnusedRandomPort-Failure/ Thanks, Jim On 12/05/2012 02:45 PM, Stuart Marks wrote: On 12/5/12 8:41 AM, Jim Gish wrote: BTW printStackTrace() prints to standard error by default -- that's why I don't explicitly have it in there. Oh yes, so it does. Sorry, I was confused. s'marks -- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.g...@oracle.com
Re: RFR: 8004317 TestLibrary.getUnusedRandomPort() fails intermittently, but exception not reported
Thanks Stuart. Sure - go ahead and make the change and do the push. Maybe we'll get lucky with the nightlies! Thanks again, Jim On 12/05/2012 06:54 PM, Stuart Marks wrote: OK, looks better, more explicit so that we can find out why this is failing. There's still a subtle issue in the reporting though. Consider if on attempt N the ServerSocket call gets a valid port but it's one of the reserved ports. Then, unusedRandomPort will be = 0 and isReservedPort() will be true, so we'll get the INFO message. Now on attempt N+1 suppose ServerSocket throws an exception. We'll get the exception stack trace, but then unusedRandomPort will still have its previous value, and we'll get the INFO message again, but spuriously this time. I hate to ask you to update this again, but as it stands I think the output will be quite confusing. I think setting unusedRandomPort back to -1 at the top of the loop should fix it. You need me to push this for you? I can drop in this change before I push, if you're OK with me doing this. s'marks On 12/5/12 12:51 PM, Jim Gish wrote: Here's a new version for your consideration :-) http://cr.openjdk.java.net/~jgish/Bug8004317-TestLibrary-getUnusedRandomPort-Failure/ http://cr.openjdk.java.net/%7Ejgish/Bug8004317-TestLibrary-getUnusedRandomPort-Failure/ Thanks, Jim On 12/05/2012 02:45 PM, Stuart Marks wrote: On 12/5/12 8:41 AM, Jim Gish wrote: BTW printStackTrace() prints to standard error by default -- that's why I don't explicitly have it in there. Oh yes, so it does. Sorry, I was confused. s'marks -- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.g...@oracle.com -- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.g...@oracle.com
Review request: 8003562: Provide a command-line tool to find static dependencies
This review request (add a new jdeps tool in the JDK) include changes in langtools and the jdk build. I would need reviewers from the langtools and the build-infra team. Webrev for review: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/langtools-webrev.02/ http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/jdk-webrev.02/ The jdeps source is in the langtools repo. The change in the jdk repo is to add the new jdeps executable. I made change in the old and new build for the addition of this new jdeps tool. I discussed with Jon whether I should modify langtools/make/build.xml to add a new jdeps target. Since the old build will be replaced by the new build soon, I simply add com/sun/tools/jdeps as part of javap.includes. Alan, The -d option is kept as a hidden option and use as implementation. We can remove it when it's determined not useful in the future. I also rename -v:summary to -summary. Thanks Mandy $ jdep -h Usage: jdeps options files where possible options include: -version Version information -classpath pathSpecify where to find class files -summary Print dependency summary only -v:class Print class-level dependencies -v:package Print package-level dependencies -p package nameRestrict analysis to classes in this package (may be given multiple times) -e regex Restrict analysis to packages matching pattern (-p and -e are exclusive) -P --profileShow profile or the file containing a package -R --recursive Traverse all dependencies recursively -all Process all classes specified in -classpath $ jdep Notepad.jar Ensemble.jar Notepad.jar - D:\tools\devtools\jdk8\windows-i586\jre\lib\rt.jar unnamed (Notepad.jar) - java.awt - java.awt.event - java.beans - java.io - java.lang - java.net - java.util - java.util.logging - javax.swing - javax.swing.border - javax.swing.event - javax.swing.text - javax.swing.tree - javax.swing.undo Ensemble.jar - D:\tools\devtools\jdk8\windows-i586\jre\lib\jfxrt.jar Ensemble.jar - D:\tools\devtools\jdk8\windows-i586\jre\lib\rt.jar com.javafx.main (Ensemble.jar) - java.applet - java.awt - java.awt.event - java.io - java.lang - java.lang.reflect - java.net - java.security - java.util - java.util.jar - javax.swing - sun.misc JDK internal API (rt.jar)
Re: RFR: 8003246: Add Supplier to ThreadLocal
On 6/12/2012 5:39 AM, Akhil Arora wrote: This patch adds a constructor to ThreadLocal to supply initial values using the new Supplier functional interface. Please review. This work was done by Jim Gish. http://cr.openjdk.java.net/~akhil/8003246.0/webrev/ http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003246 Has anyone actually established that this is a useful addition to make? What is the acceptance criteria for adding these new mechanisms to existing classes? Was there a CCC request for this? We need to be very wary of unnecessary bloat. David Thanks
hg: jdk8/tl/jdk: 8004317: TestLibrary.getUnusedRandomPort() fails intermittently, but exception not reported
Changeset: a971516029ab Author:jgish Date: 2012-12-05 21:08 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a971516029ab 8004317: TestLibrary.getUnusedRandomPort() fails intermittently, but exception not reported Reviewed-by: alanb, dmocek, smarks ! test/java/rmi/testlibrary/TestLibrary.java
Re: RFR: 8004317 TestLibrary.getUnusedRandomPort() fails intermittently, but exception not reported
Pushed: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a971516029ab I couldn't resist doing a couple more fixups. (Man, there is a lot more cleaning up that could be done in here.) I don't know if I got this in time for tonight's nightly. Well, get it in there and see if it falls over sooner or later. s'marks On 12/5/12 4:02 PM, Jim Gish wrote: Thanks Stuart. Sure - go ahead and make the change and do the push. Maybe we'll get lucky with the nightlies! Thanks again, Jim On 12/05/2012 06:54 PM, Stuart Marks wrote: OK, looks better, more explicit so that we can find out why this is failing. There's still a subtle issue in the reporting though. Consider if on attempt N the ServerSocket call gets a valid port but it's one of the reserved ports. Then, unusedRandomPort will be = 0 and isReservedPort() will be true, so we'll get the INFO message. Now on attempt N+1 suppose ServerSocket throws an exception. We'll get the exception stack trace, but then unusedRandomPort will still have its previous value, and we'll get the INFO message again, but spuriously this time. I hate to ask you to update this again, but as it stands I think the output will be quite confusing. I think setting unusedRandomPort back to -1 at the top of the loop should fix it. You need me to push this for you? I can drop in this change before I push, if you're OK with me doing this. s'marks On 12/5/12 12:51 PM, Jim Gish wrote: Here's a new version for your consideration :-) http://cr.openjdk.java.net/~jgish/Bug8004317-TestLibrary-getUnusedRandomPort-Failure/ http://cr.openjdk.java.net/%7Ejgish/Bug8004317-TestLibrary-getUnusedRandomPort-Failure/ Thanks, Jim On 12/05/2012 02:45 PM, Stuart Marks wrote: On 12/5/12 8:41 AM, Jim Gish wrote: BTW printStackTrace() prints to standard error by default -- that's why I don't explicitly have it in there. Oh yes, so it does. Sorry, I was confused. s'marks -- Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304 Oracle Java Platform Group | Core Libraries Team 35 Network Drive Burlington, MA 01803 jim.g...@oracle.com
Re: Review request: 8004042 : Arrrghs.java test failed on windows with access error.
On 12/5/12 2:44 PM, David DeHaven wrote: A fix for intermittent test failures causing grief on Windows, particularly Windows 7 and 2008 systems: http://cr.openjdk.java.net/~ddehaven/8004042/webrev.1/ The underlying cause is as of yet unknown, but I was able to create an environment that caused similar failures by running watch -n 0.2 cat JTwork/scratch/atest.bat in another shell then running jtreg. Without the patch it would fail very regularly (especially when isolated and looped 1,000 times), with the patch it always passes. We had been discussing/speculating that the problem is the virus scanner checking the old incarnation of the file at the same time we want to create a new one. If so, and if it were to hold the file open with exclusive access, that would explain the AccessDeniedException instead of FileAlreadyExistsException (in the CREATE_NEW case). (I still don't have an explanation for why the file deletion doesn't throw an exception. That's what seems to cause jtreg some grief in its cleanup phase, and for that reason we've added retry logic to jtreg.) In any case I think backing off and retrying is probably what's getting us the benefit, not the adjustment of the file creation flags. In any case I'd increase the number of retry attempts. The test environment is surprisingly hostile. In the RMI tests we try to open an unused network port and retry 10 times if that fails, and sometimes that's not enough. (See Jim Gish's recent changeset.) InterruptedException shouldn't be ignored. Jtreg will interrupt the test if it times out, so this interruption should be handled gracefully. Perhaps, wrap the InterruptedException in an IOException and rethrow it? (Since the caller is clearly prepared to handle IOException.) Terminating the loop and returning normally doesn't seem right, since the file wasn't created successfully. This is only style, but perhaps it would be good to get rid of the 'done' boolean and replace the 'done = true' statement with a 'return'. This simplifies things a bit, I think. s'marks
Re: Request for Review : CR#8004015 : [final (?) pass] Add interface extends and defaults for basic functional interfaces
On 6/12/2012 4:20 AM, Mike Duigou wrote:
I have updated webrev again to fix some reported javadoc technical issues and
added null handling specification to the {Int|Double|Long}Supplier.
http://cr.openjdk.java.net/~mduigou/8004015/2/webrev/
http://cr.openjdk.java.net/~mduigou/8004015/2/specdiff/java/util/function/package-summary.html
I believe that this iteration is complete (or very nearly so).
Sorry to be a pain but this:
left - the left operand, must be non-null
doesn't tell you what happens if it is null. Is it not better to simply
have:
@param left the left operand
@param right the right operand
@throws NullPointerException if either left or right are null
?
David
-
Mike
On Dec 4 2012, at 21:47 , Mike Duigou wrote:
Hello all;
I have updated the proposed patch. The changes primarily add class and method
documentation regarding handling of null for the primitive specializations.
http://cr.openjdk.java.net/~mduigou/8004015/1/webrev/
http://cr.openjdk.java.net/~mduigou/8004015/1/specdiff/java/util/function/package-summary.html
I've also reformatted the source for the default methods.
Mike
On Nov 26 2012, at 18:12 , Mike Duigou wrote:
Hello all;
In the original patch which added the basic lambda functional interfaces,
CR#8001634 [1], none of the interfaces extended other interfaces. The reason
was primarily that the javac compiler did not, at the time that 8001634 was
proposed, support extension methods. The compiler now supports adding of method
defaults so this patch improves the functional interfaces by filing in the
inheritance hierarchy.
Adding the parent interfaces and default methods allows each functional
interface to be used in more places. It is especially important for the
functional interfaces which support primitive types, IntSupplier, IntFunction,
IntUnaryOperator, IntBinaryOperator, etc. We expect that eventually standard
implementations of these interfaces will be provided for functions like max,
min, sum, etc. By extending the reference oriented functional interfaces such
as Function, the primitive implementations can be used with the boxed primitive
types along with the primitive types for which they are defined.
The patch to add parent interfaces and default methods can be found here:
http://cr.openjdk.java.net/~mduigou/8004015/0/webrev/
http://cr.openjdk.java.net/~mduigou/8004015/0/specdiff/java/util/function/package-summary.html
Mike
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c2e80176a697
Request for Review: CR#8001667, second attempt
Hi, This update reflect changes based on feedbacks for last version, the changes are - rename reverse() to reverseOrder() - rename Comparator.compose to Comparator.thenComparing - add 4 Comparator.thenComparing methods take [Int|Long|Double]Function to enable fluent syntax like this example, people.sort(Comparators.comparing(People::getFirstName).thenComparing(People.getLastName)) vs previously, people.sort(Comparators.comparing(Person::getName)) ComparatorPerson byLastFirst = Comparators.comparing(Person::getLastName) .compose(Comparators.comparing(Person::getFirstName)) Please review and comment on the webrev[1] and specdiff[2]. [1] http://cr.openjdk.java.net/~henryjen/ccc/8001667.1/webrev [2] http://cr.openjdk.java.net/~henryjen/ccc/8001667.1/specdiff/overview-summary.html Cheers, Henry
