Re: Code Review Request: 7160252: (prefs) NodeAddedEvent was not delivered when new node add when new Node
Thanks Kurchi, looks fine. -Chris Kurchi Hazra kurchi.subhra.ha...@oracle.com wrote: On 7/11/12 4:24 PM, Chris Hegarty wrote: On 12 Jul 2012, at 00:15, Kurchi Hazrakurchi.subhra.ha...@oracle.com wrote: Thanks for the review Alan. Updated webrev: http://cr.openjdk.java.net/~khazra/7160252/webrev.01/ Looks fine. Trivially, is there an opportunity to make any fields final since initFields is replaced with a constructor? Thanks for pointing that out. How about: http://cr.openjdk.java.net/~khazra/7160252/webrev.02/ - Kurchi -Chris. - Kurchi On 7/11/12 5:45 AM, Alan Bateman wrote: On 26/06/2012 22:57, Kurchi Hazra wrote: Hi, On Mac OS X, for Preferences, a new child added event was not being delivered to a NodeChangeListener since the existing code depended on the return value of addNode() in the native code, which returns true if a new node is added. However, since addNode() was being called erroneously after a child node is already added to an existing node, addNode() would always return false, resulting in thw new node event never being delivered. This fix propagates the required information of whether a node is added from the method adding the child node itself. In addition, I cleaned up the constructors in MacOSXPreferences.java and added a test (AddNodeChangeListener.java) to cover this case. Finally, there were two prefs tests in ProblemList.txt which are now passing, I have removed these from the ProblemList too. Bug: http://bugs.sun.com/view_bug.do?bug_id=7160252 Webrev: http://cr.openjdk.java.net/~khazra/7160252/webrev.00/ Thanks, Kurchi It doesn't look you got a reviewer for this one. I looked at the changes and the fix seems okay to me. The only odd thing is that now that you have the 5-arg constructor then it can initialize all the fields allowing you to get rid of initFields and also setNew. Also I assume it means you can close 7150557. -Alan.
Re: [PATCH] Sunbug 7131192: Optimize BigInteger.doubleValue(), floatValue()
On 07/11/2012 11:05 PM, Louis Wasserman wrote: Doing it with bit-twiddling and Double.longBitsToDouble improves the speed of those methods by something like two orders of magnitude in my benchmarks. Mmm, but that's going to hit double-rounding bugs for large numbers. Where is this patch, anyway? Andrew.
Re: Code Review Request: 7160252: (prefs) NodeAddedEvent was not delivered when new node add when new Node
On 12/07/2012 00:45, Kurchi Hazra wrote: On 7/11/12 4:24 PM, Chris Hegarty wrote: On 12 Jul 2012, at 00:15, Kurchi Hazrakurchi.subhra.ha...@oracle.com wrote: Thanks for the review Alan. Updated webrev: http://cr.openjdk.java.net/~khazra/7160252/webrev.01/ Looks fine. Trivially, is there an opportunity to make any fields final since initFields is replaced with a constructor? Thanks for pointing that out. How about: http://cr.openjdk.java.net/~khazra/7160252/webrev.02/ Looks fine except that the test case is missing from latest webrev. Assuming that test/java/util/prefs/AddNodeChangeListener.java hasn't changed from the original webrev then I think you are all set to push. -Alan.
Re: [PATCH] Sunbug 7131192: Optimize BigInteger.doubleValue(), floatValue()
It was attached to the previous message? I don't know if this list works with attachments. Alternately, the patch was attached here: https://bugs.openjdk.java.net/show_bug.cgi?id=100222 I'm not sure what you mean by double-rounding bugs, though. It's not difficult to actually implement the HALF_EVEN rounding behavior with bit twiddling. That said, I *did* encounter places where prior erroneous behavior (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6358355) of Float.parseFloat caused my correct implementation to fail tests... Louis Wasserman wasserman.lo...@gmail.com http://profiles.google.com/wasserman.louis On Thu, Jul 12, 2012 at 7:58 AM, Andrew Haley a...@redhat.com wrote: On 07/11/2012 11:05 PM, Louis Wasserman wrote: Doing it with bit-twiddling and Double.longBitsToDouble improves the speed of those methods by something like two orders of magnitude in my benchmarks. Mmm, but that's going to hit double-rounding bugs for large numbers. Where is this patch, anyway? Andrew.
Re: RFR [7u8]: 7166896: DocumentBuilder.parse(String uri) is not IPv6 enabled. It throws MalformedURLException
Hi Joe, On Jul 11, 2012, at 7:59 AM, Joe Wang wrote: Hi Paul, This is now for 7u8, so I started a new thread. As we discussed, I've removed the hack of using escapeNonUSAscii. In this regard, we're now in sync with the source code in Xerces. Here's the webrev: http://cr.openjdk.java.net/~joehw/7u8/7166896/webrev/ Looks OK to me. Note i am not an official reviewer, yet! so please don't commit cause i thought it looked OK ;-) Paul.
Re: [PATCH] Sunbug 7131192: Optimize BigInteger.doubleValue(), floatValue()
On 07/12/2012 10:32 AM, Louis Wasserman wrote: It was attached to the previous message? I don't know if this list works with attachments. Alternately, the patch was attached here: https://bugs.openjdk.java.net/show_bug.cgi?id=100222 I'm not sure what you mean by double-rounding bugs, though. It's not difficult to actually implement the HALF_EVEN rounding behavior with bit twiddling. Sure, as long as you've thought about it and done it carefully. The bit twiddling is easy to do, and easy to get wrong. From the supplied patch it looks like you've done a good job, but there was no way to tell without it. I presume the listserv dropped it on the floor. Andrew.
Re: RFR [7u8]: 7166896: DocumentBuilder.parse(String uri) is not IPv6 enabled. It throws MalformedURLException
+1 Best Lance On Jul 12, 2012, at 5:42 AM, Paul Sandoz wrote: Hi Joe, On Jul 11, 2012, at 7:59 AM, Joe Wang wrote: Hi Paul, This is now for 7u8, so I started a new thread. As we discussed, I've removed the hack of using escapeNonUSAscii. In this regard, we're now in sync with the source code in Xerces. Here's the webrev: http://cr.openjdk.java.net/~joehw/7u8/7166896/webrev/ Looks OK to me. Note i am not an official reviewer, yet! so please don't commit cause i thought it looked OK ;-) Paul. Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
[PATCH] Sunbug 6358355: Rounding error in Float.parseFloat
Float.parseFloat doesn't currently round correctly, or even monotonically,
in certain cases.
In particular, the following test program prints false:
public class Foo {
public static void main(String[] args) {
System.out.println(144115196665790480f = 144115196665790481f);
}
}
A patch is attached, and can also be found at
https://bugs.openjdk.java.net/show_bug.cgi?id=100208.
There was a comment in sun.misc.FloatingDecimal claiming this would take
400 lines of code, but by eliminating the (fallacious) sticky rounding
logic, and just duplicating the double-parsing logic, it only ends up
costing ~40 net lines of code added.
The added code is mostly identical to the preexisting double-parsing code.
This is a prerequisite for the separate, previously sent patch improving
the performance of BigInteger.doubleValue() and floatValue(). (Testing for
that patch revealed this bug.)
Louis Wasserman
wasserman.lo...@gmail.com
http://profiles.google.com/wasserman.louis
Re: Code Review Request: 7160252: (prefs) NodeAddedEvent was not delivered when new node add when new Node
I used a new workspace and missed adding it to mercurial. The test remains the same, I'll push it after adding the test. - Kurchi On 7/12/12 12:16 AM, Alan Bateman wrote: On 12/07/2012 00:45, Kurchi Hazra wrote: On 7/11/12 4:24 PM, Chris Hegarty wrote: On 12 Jul 2012, at 00:15, Kurchi Hazrakurchi.subhra.ha...@oracle.com wrote: Thanks for the review Alan. Updated webrev: http://cr.openjdk.java.net/~khazra/7160252/webrev.01/ Looks fine. Trivially, is there an opportunity to make any fields final since initFields is replaced with a constructor? Thanks for pointing that out. How about: http://cr.openjdk.java.net/~khazra/7160252/webrev.02/ Looks fine except that the test case is missing from latest webrev. Assuming that test/java/util/prefs/AddNodeChangeListener.java hasn't changed from the original webrev then I think you are all set to push. -Alan.
Re: RFR [7u8]: 7166896: DocumentBuilder.parse(String uri) is not IPv6 enabled. It throws MalformedURLException
Lance, Paul, Thanks! Joe On 7/12/2012 3:55 AM, Lance Andersen - Oracle wrote: +1 Best Lance On Jul 12, 2012, at 5:42 AM, Paul Sandoz wrote: Hi Joe, On Jul 11, 2012, at 7:59 AM, Joe Wang wrote: Hi Paul, This is now for 7u8, so I started a new thread. As we discussed, I've removed the hack of using escapeNonUSAscii. In this regard, we're now in sync with the source code in Xerces. Here's the webrev: http://cr.openjdk.java.net/~joehw/7u8/7166896/webrev/ Looks OK to me. Note i am not an official reviewer, yet! so please don't commit cause i thought it looked OK ;-) Paul. 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: Code Review Request 7142596: RMI JPRT tests are failing
OK, I took a look at the revised webrev. Most stuff is good. A couple changes are necessary though. *** MultipleRegistries.java Not a big deal, but the comment at lines 65-67 is no longer necessary. *** TestLibrary.java I think the reserved port range stuff still needs to be fixed up. The comment at lines 81-82 talks about a range 64000-64100 which isn't really present anywhere. The comment should instead say that PORT_MIN and PORT_MAX should be kept adjusted so that they specify the range of ports defined by the constants following. Make sure to do this adjustment; PORT_MAX is 64002 here. The previous webrev had PORT_MIN and PORT_MAX specify a range of 1024-64000 as the *allowed* range for ports returned by getUnusedRandomPort(). But as I described previously, this is difficult to enforce given the varying port allocation schemes on different OSes. Instead I think we need to invert the sense of the range PORT_MIN..PORT_MAX and make it be the range of ports reserved by tests that require fixed ports (64001 through 64005). Then, getUnusedRandomPort() needs to be changed to *disallow* ports in this range, that is, to retry if the port is within this range. Thus the condition on the while-loop has to be inverted, something like while (... unusedRandomPort = PORT_MIN unusedRandomPort = PORT_MAX) So, how could getUnusedRandomPort() have worked in its present state? Well I think it ends up retrying 10 times, then at the end of the loop unusedRandomPort is actually some legal port number -- albeit possibly one within the disallowed range -- and almost certainly not -1, and so the method returns it and the caller uses it. So, the success-testing logic here will also have to change so that it retests whether the port is in the disallowed range. The comment on getUnusedRandomPort() also needs to be updated to reflect its new policy on the reserved range, as well as throwing an exception on failure. Also (nitpick) it should say less than not less then. Looking at getUnusedRandomPort() again more closely [sorry] I think the catch of Exception that does nothing is suspicious. I'm not sure why getting a ServerSocket would fail, but if it does (maybe the system is out of ports??) we should just throw out to the caller. Perhaps ideally we'd just have this method throw IOException, but if that requires all callers to be updated, it might be easier just to catch IOException and throw a RuntimeException that wraps the caught IOException. Similar comments apply to the catch(Exception)/do-nothing code in the other utility methods. Certainly getRegistryPort() should just throw (or possibly wrap and rethrow) any exception caught. For createRegistryOnUnusedPort(), the catching of ExportException is handled properly. The second catch clause of the outer try-block, and the catch of the inner try-block, both ignore exceptions. The code will then end up retrying. Is it reasonable that retrying in these cases will result in a different outcome? My guess is that it's more likely that something is seriously wrong that will cause all the retries to fail, in which case this method will discard the exceptions it has just caught and throw a *new* RemoteException instance. I'm particularly sensitive to this; as you might recall a couple weeks ago I was having a wrestling match with the jstatd tests (I finished the match, but I'm not sure I won). The primary problem was that several layers of code would catch and discard exceptions, which made diagnosing the problem incredibly difficult. So, I'd recommend removing the do nothing catch clauses. Thinking further about createRegistryOnUnusedPort(), I'm not sure that retrying 10 (or some other number of times) actually makes sense. It does for getUnusedRandomPort(), which has to retry in order to get a port outside the disallowed range. But for creating a registry, createRegistry(0) will usually work the first time. If it throws ExportException, it does so for a specific reason, so we should retry once on an unused random port. But if this still fails, don't think retrying repeatedly makes sense. s'marks On 7/10/12 2:14 PM, Darryl Mocek wrote: On 07/09/2012 04:41 PM, Stuart Marks wrote: OK, here's the review for the second half of the files in the webrev. I saw your reply to the first half (to which I'll reply separately), and I don't think there's anything here that's affected by them. *** AppleUserImpl.java *** ApplicationServer.java REGISTRY_PORT should be a local variable; also rename to use mixed case. Changed to a private registryPort (see next issue). Eh, whoops, after looking at ApplicationServer.java I see that it accesses the REGISTRY_PORT field directly. This is why direct field access is a bad idea. :-) Now the question is, has REGISTRY_PORT been initialized before ApplicationServer needs it? It turns out that it has been -- but only in some cases. It seems like the test is trying to
hg: jdk8/tl/jaxp: 7183760: DocumentBuilder.parse(String uri) is not IPv6 enabled
Changeset: 6e444b892c99 Author:joehw Date: 2012-07-12 21:06 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jaxp/rev/6e444b892c99 7183760: DocumentBuilder.parse(String uri) is not IPv6 enabled Summary: removing the hack of using escapeNonUSAscii. this is the same patch as 7166896 for 7u8. Reviewed-by: psandoz, lancea ! src/com/sun/org/apache/xerces/internal/impl/XMLEntityManager.java
