Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Thank you Ulf!
On 19.03.2014 2:12, Ulf Zibis wrote:
Am 18.03.2014 19:28, schrieb Ivan Gerasimov:
Assuming this last iteration is OK, should the next step be a CCC
request?
Do you mean? :
/*
* ...
+ * It is assumed that fromIndex = toIndex, otherwise the
behaviour of this method is undefined.
* ...
- * toIndex fromIndex})
* ...
*/
protected void removeRange(int fromIndex, int toIndex) {
...
The (fromIndex toIndex) condition is checked now, so the behavior of
the method is defined - it throws an exception.
Please remove and replace inline by size - toIndex:
621 int numMoved = size - toIndex;
It is done this way throughout the class.
I don't think it has to be changed in this particular place.
Improving modCound handling can be done with a different RFE I guess, as
it doesn't connected to the rest of the fix.
Sincerely yours,
Ivan
About modCount:
Wouldn't it be more correct to code? :
616 if (fromIndex toIndex) {
617 throw new IndexOutOfBoundsException(
618 outOfBoundsMsg(fromIndex, toIndex));
619 }
620 try {
621 modCount++;
622 System.arraycopy(elementData, toIndex, elementData,
fromIndex,
623 size - toIndex);
624 } catch (IndexOutOfBoundsException ioobe) {
625 modCount--;
626 throw ioobe;
627 }
Of even better :
000 private int[] modCount = { 0 };
...
616 if (fromIndex toIndex) {
617 throw new IndexOutOfBoundsException(
618 outOfBoundsMsg(fromIndex, toIndex));
619 }
620 System.arraycopy(elementData, toIndex, elementData,
fromIndex,
621 size - toIndex, modCount);
Or :
000 public class ArrayList ... implements ModCounter {
001 private int modCount = 0;
001 void incModCount() {
001 modCount++;
004}
...
616 if (fromIndex toIndex) {
617 throw new IndexOutOfBoundsException(
618 outOfBoundsMsg(fromIndex, toIndex));
619 }
620 System.arraycopy(elementData, toIndex, elementData,
fromIndex,
621 size - toIndex, this);
-Ulf
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Fine by me. David On 19/03/2014 4:37 AM, Ivan Gerasimov wrote: Sorry, here's the link: http://cr.openjdk.java.net/~igerasim/8014066/4/webrev/ On 18.03.2014 22:28, Ivan Gerasimov wrote: Hello! Would you please take a look at the next iteration of webrev? I incorporated the last suggestions in it. When I first proposed a simple typo fix, I didn't think it's going to cause such a big discussion :) Assuming this last iteration is OK, should the next step be a CCC request? Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 19/03/14 08:29, David Holmes wrote: Fine by me. David On 19/03/2014 4:37 AM, Ivan Gerasimov wrote: Sorry, here's the link: http://cr.openjdk.java.net/~igerasim/8014066/4/webrev/ Looks fine to me too. -Chris. On 18.03.2014 22:28, Ivan Gerasimov wrote: Hello! Would you please take a look at the next iteration of webrev? I incorporated the last suggestions in it. When I first proposed a simple typo fix, I didn't think it's going to cause such a big discussion :) Assuming this last iteration is OK, should the next step be a CCC request? Sincerely yours, Ivan
Re: RFR: JDK-8035099 LocalTime with(MILLI_OF_DAY/MICRO_OF_DAY) incorrect
On 17/03/2014 14:51, roger riggs wrote: Hi, This looks fine (not a Reviewer). Looks okay to me too. I'm checking on how to handle the change in TCK tests. The same question (and answer) applies to JDK-8036818: DateTimeFormatter withResolverFields() fails to accept null There is a process for challenging conformance tests so I wouldn't expect any issues. -Alan
Re: RFR: JDK-8036785 ChronoLocalDate refers to generics that have been removed
On 12/03/2014 10:52, Stephen Colebourne wrote: This is a request for review of this bug: https://bugs.openjdk.java.net/browse/JDK-8036785 During development, ChronoLocalDate had a generic type parameter. It was removed during the development of JSR-310. The Javadoc was not updated to reflect the removal. The necessary change is to text that is essentially non-normative, and as such this change should be non-controversial. Have you considered using @apiNote here? -Alan.
Re: RFR: JDK-8036785 ChronoLocalDate refers to generics that have been removed
At the time it was originally written I don't think @apiNote existed. I agree it would be good to get the separation in there. However my current concern is getting the change back to jdk8u, and it seems that the simplest solution might be the best to achieve that. Perhaps later, I might then consider rechecking all of java.time for non-normative stuff to go in a separate commit. Stephen On 19 March 2014 10:53, Alan Bateman alan.bate...@oracle.com wrote: On 12/03/2014 10:52, Stephen Colebourne wrote: This is a request for review of this bug: https://bugs.openjdk.java.net/browse/JDK-8036785 During development, ChronoLocalDate had a generic type parameter. It was removed during the development of JSR-310. The Javadoc was not updated to reflect the removal. The necessary change is to text that is essentially non-normative, and as such this change should be non-controversial. Have you considered using @apiNote here? -Alan.
Re: RFR: JDK-8036785 ChronoLocalDate refers to generics that have been removed
On 19/03/2014 10:59, Stephen Colebourne wrote: At the time it was originally written I don't think @apiNote existed. I agree it would be good to get the separation in there. However my current concern is getting the change back to jdk8u, and it seems that the simplest solution might be the best to achieve that. Perhaps later, I might then consider rechecking all of java.time for non-normative stuff to go in a separate commit. I'd like to us make use of the the new tags in other areas, especially paragraphs and statements where it might not be initially clear where they are normative or non-normative. In any case, the patch looks okay to me and I think Roger has said he would sponsor it. -Alan
Re: JDK-8036003: Add variable not to separate debug information.
On 03/18/2014 06:22 PM, Andrew Hughes wrote: The intent was for #3 to cover this case (i.e. whatever Oracle does now) and for #2 to be what the GNU/Linux distributions want (i.e. binaries with all debuginfo generated and left intact so they can do their own stripping). Mmm, but maybe that will break things when debuginfo isn't installed. In fact, I already know that it has broken some things. So it's not ideal. Andrew.
Re: JDK-8036003: Add variable not to separate debug information.
On 2014-03-18 19:25, Andrew Haley wrote: On 03/18/2014 06:22 PM, Andrew Hughes wrote: The intent was for #3 to cover this case (i.e. whatever Oracle does now) and for #2 to be what the GNU/Linux distributions want (i.e. binaries with all debuginfo generated and left intact so they can do their own stripping). Mmm, but maybe that will break things when debuginfo isn't installed. In fact, I already know that it has broken some things. So it's not ideal. Which case is it that will break? #3 or #2? What would an ideal solution be for you? /Magnus
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Thanks Alan! These two tests used the commands run from non very common location (/usr/bin/ instead of /bin/), so I suspect they have been rarely run. As it follows from the summaries, one of them ensures the VM doesn't crash; the other checks, whether the input/output streams are left open. Both tests in the case of a failure could interfere with other tests unless they run in the othervm mode. That's why I thought it's better to add the flag. If a test is badly behaved and is leaving streams open then I would agree with othervm. However these are old issues (right?) and should not be happening now. Also if a test tickles a bug that causes the VM to crash then jtreg will spin up a new VM for the next test. So if it's possible to avoid othervm then we should (and from what I can tell then these tests have been well behaved when running without othervm before). Yes, got it. I will remove othervm mode. Once I was suggested to add the othervm mode to the test which was to detect a memory leak (in the case of a poor behavior). So I was under wrong impression that we need to use this mode even when normal run of a test doesn't influence any other. Omitting othervm for the tests that do not interfere with others during normal runs does make sense, and I will follow this rule from now on. IMO ideally, there should be a configurable part of the harness, where all the shell commands are set up. So that they could be accessed by both Java and shell-based regtests. test/lib/testlibrary is the place for test-suite wide infrastructure. I don't know if there are tests beyond the Process area that needs to do the same kind of thing. I meant something that can be configured by a user of the harness. It's beyond the scope of this bug, of course. Sincerely yours, Ivan
Re: JDK-8036003: Add variable not to separate debug information.
On 03/19/2014 01:51 PM, Magnus Ihse Bursie wrote: On 2014-03-18 19:25, Andrew Haley wrote: On 03/18/2014 06:22 PM, Andrew Hughes wrote: The intent was for #3 to cover this case (i.e. whatever Oracle does now) and for #2 to be what the GNU/Linux distributions want (i.e. binaries with all debuginfo generated and left intact so they can do their own stripping). Mmm, but maybe that will break things when debuginfo isn't installed. In fact, I already know that it has broken some things. So it's not ideal. Which case is it that will break? #3 or #2? What would an ideal solution be for you? I think the problem was that jstack and jmap didn't work until the debuginfo package was installed. This tells me that the debuginfo stripping in our Linux build system is too aggressive for this purpose. I think that's something we must fix ourselves. What we really need from OpenJDK is a way to build with complete debuginfo in both binaries and jarfiles. Andrew.
Re: JDK-8036003: Add variable not to separate debug information.
- Original Message - On 03/19/2014 01:51 PM, Magnus Ihse Bursie wrote: On 2014-03-18 19:25, Andrew Haley wrote: On 03/18/2014 06:22 PM, Andrew Hughes wrote: The intent was for #3 to cover this case (i.e. whatever Oracle does now) and for #2 to be what the GNU/Linux distributions want (i.e. binaries with all debuginfo generated and left intact so they can do their own stripping). Mmm, but maybe that will break things when debuginfo isn't installed. In fact, I already know that it has broken some things. So it's not ideal. Which case is it that will break? #3 or #2? What would an ideal solution be for you? I think the problem was that jstack and jmap didn't work until the debuginfo package was installed. This tells me that the debuginfo stripping in our Linux build system is too aggressive for this purpose. Yes, https://bugzilla.redhat.com/show_bug.cgi?id=1010786 The distro builds strip too much and remove the needed symtab from libjvm.so. I think that's something we must fix ourselves. What we really need from OpenJDK is a way to build with complete debuginfo in both binaries and jarfiles. This was my intent with #2. The jstack/jmap issue needs to be fixed by stripping less debuginfo post-build on libjvm.so. Andrew. -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Here's the (hopefully final) polished webrev: removed 'othervm' and replaced stderr with stdout in reporting of the wrong OS. Would you please take a look? http://cr.openjdk.java.net/~igerasim/6943190/4/webrev/ Sincerely yours, Ivan
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
On 19.03.2014 20:11, Martin Buchholz wrote: On Wed, Mar 19, 2014 at 1:19 AM, Ivan Gerasimov ivan.gerasi...@oracle.com mailto:ivan.gerasi...@oracle.com wrote: Improving modCound handling can be done with a different RFE I guess, as it doesn't connected to the rest of the fix. I don't think modCount handling needs improvement. I didn't mean to propose modCount improvement. What I really meant was that modCount improvement (if required) should be separated from the removeRange's spec fix. If you're actually looking for follow-on improvements, I suggest globally replacing all of my Fun() poor-man emulations of lambdas with Real Lambdas. Thanks for suggestion, Martin. I crated an issue to track this: https://bugs.openjdk.java.net/browse/JDK-8037866 Sincerely yours, Ivan
Re: StringBuilder version of java.util.regex.Matcher.append*
Similar suggestion has been on the to-do list for a while. There were discussion back then that it might be interesting to add the more general interface Appendable... The issue was how to deal with the IOE declared by the Appendable methods then. If the appendable is not going to happen, then it is definitely worth adding the StringBuilder for better performance. -Sherman On 03/19/2014 10:51 AM, Jeremy Manson wrote: I'm told that the diff didn't make it. I've put it in a Google drive folder... https://drive.google.com/file/d/0B_GaXa6O4K5LY3Y0aHpranM3aEU/edit?usp=sharing Jeremy On Wed, Mar 19, 2014 at 10:41 AM, Jeremy Mansonjeremyman...@google.comwrote: Hi folks, We've had this internally for a while, and I keep meaning to bring it up here. The Matcher class has a few public methods that take StringBuffers, and we've found it useful to add similar versions that take StringBuilders. It has two benefits: - Users don't have to convert from one to the other when they want to use the method in question. The symmetry is nice. - The StringBuilder variants are faster (if lock optimizations don't kick in, which happens in the interpreter and the client compiler). For interpreted / client-compiled code, we saw something like a 25% speedup on String.replaceAll(), which calls into this code. Any interest? Diff attached. Jeremy
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
Thanks Martin!
On 19.03.2014 20:23, Martin Buchholz wrote:
I expected to see System.out used instead (not that it matters much).
I would write:
System.out.println('true' command not found; skipping test);
Two cases are mixed here:
- normal run (OS does not have this command),
- erroneous run (OS isn't configured correctly, so the command couldn't
be find).
I agree it's not matters much, so no problem to replace the output as
you suggest.
Here's the (final?) webrev:
http://cr.openjdk.java.net/~igerasim/6943190/5/webrev/
Sincerely yours,
Ivan
Re: AWT Dev [9] Review request: new macro for conversion to jboolean
Thanks Anthony! Can somebody from the core-libs team take a look? On 3/17/14 10:31 PM, Anthony Petrov wrote: Personally, I'd call it to_jboolean(obj), but IS_TRUE(obj) sounds good to me too. Either way, I'm fine with the fix. -- best regards, Anthony On 3/17/2014 7:01 PM, Sergey Bylokhov wrote: Hello. This review request is for the new macro, which simplify conversion to jboolean. It will be useful for fixing parfait warnings. We have a lot of places, where we cast some type to jboolean: BOOL = retVal; return (jboolean) retVal; WARNING: Expecting value of JNI primitive type jboolean: mismatched value retVal with size 32 bits, retVal used for conversion to int8 in return +++ b/src/share/native/common/jni_util.hMon Mar 17 18:28:48 2014 +0400 @@ -277,6 +277,7 @@ #define IS_NULL(obj) ((obj) == NULL) #define JNU_IsNull(env,obj) ((obj) == NULL) +#define IS_TRUE(obj) ((obj) ? JNI_TRUE : JNI_FALSE) I am not sure about the name, probably someone have a better suggestion? The fix is for jdk9/dev. -- Best regards, Sergey. -- Best regards, Sergey.
Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64
https://bugs.openjdk.java.net/browse/JDK-8035807 Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/ This patch converts the last 2 references to sun.misc.BASE64Encoder/Decoder from the jdk repo with java.util.Base64. We should also update the tests and I have filed JDK-8037873 for that. Thanks Mandy
Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64
Fix looks fine. You just need to update the import statements in Obj.java On 19 Mar 2014, at 18:37, Mandy Chung mandy.ch...@oracle.com wrote: https://bugs.openjdk.java.net/browse/JDK-8035807 Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/ This patch converts the last 2 references to sun.misc.BASE64Encoder/Decoder from the jdk repo with java.util.Base64. We should also update the tests and I have filed JDK-8037873 for that. Thanks Mandy
Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64
OK. Thanks. On 19 Mar 2014, at 19:06, Mandy Chung mandy.ch...@oracle.com wrote: On 3/19/2014 12:03 PM, Vincent Ryan wrote: Fix looks fine. You just need to update the import statements in Obj.java Fixed. Not sure how it's missed in the webrev :) thanks Mandy On 19 Mar 2014, at 18:37, Mandy Chung mandy.ch...@oracle.com wrote: https://bugs.openjdk.java.net/browse/JDK-8035807 Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/ This patch converts the last 2 references to sun.misc.BASE64Encoder/Decoder from the jdk repo with java.util.Base64. We should also update the tests and I have filed JDK-8037873 for that. Thanks Mandy
Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64
On 3/19/2014 12:03 PM, Vincent Ryan wrote: Fix looks fine. You just need to update the import statements in Obj.java Fixed. Not sure how it's missed in the webrev :) thanks Mandy On 19 Mar 2014, at 18:37, Mandy Chung mandy.ch...@oracle.com wrote: https://bugs.openjdk.java.net/browse/JDK-8035807 Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/ This patch converts the last 2 references to sun.misc.BASE64Encoder/Decoder from the jdk repo with java.util.Base64. We should also update the tests and I have filed JDK-8037873 for that. Thanks Mandy
Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64
On 19/03/2014 18:37, Mandy Chung wrote: https://bugs.openjdk.java.net/browse/JDK-8035807 Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/ This patch converts the last 2 references to sun.misc.BASE64Encoder/Decoder from the jdk repo with java.util.Base64. We should also update the tests and I have filed JDK-8037873 for that. Good to see more conversion to the new API, looks good to me. -Alan
Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64
On 03/19/2014 11:37 AM, Mandy Chung wrote: https://bugs.openjdk.java.net/browse/JDK-8035807 Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/ This patch converts the last 2 references to sun.misc.BASE64Encoder/Decoder from the jdk repo with java.util.Base64. We should also update the tests and I have filed JDK-8037873 for that. Thanks Mandy The sun.misc.BASE64En/Decoder is MIME type, so it outputs the \r\n per 76 characters during encoding, and ignores/skip \r or \n when decoding. The new Base64.getEncoder/Decoder() returns the basic Base64 coder, which it never inserts line separator when output, and throws exception for any non-base64- alphabet character, including \r and \n. The only disadvantage/incompatibility (j.u.Base64.getMimeDecoer() vs sun.misc.BASE64Decoder) of switching to j.u.Base64 MIME type en/decoder is that the Base64 Mime decoder ignores/skips any non-base64-alphabet (including \r and \n), while sun.misc.BASE64Decoder appears to simply use the init value -1 for any non-base64-alphabet character for decoding. I'm not familiar with the use scenario of ldap's Obj class, so I'm not sure if it matters (if it ever outputs/inputs 76 character data, or even it does,if the difference matters). Btw, except getMimeEncoder(int ...) all other Base64.getXXXEn/Decoder() returns singleton, so the de/encoder cache might not be necessary. -Sherman
Re: AWT Dev [9] Review request: new macro for conversion to jboolean
Definitely a useful macro. I too would prefer a name like TO_JBOOLEAN since it reveals the result type. Also all uppercase to identify it as a macro. If we are paranoid and want to reduce the chance of a name collision then JNU_TO_JBOOLEAN perhaps. I would also define the macro as: #define JNU_TO_JBOOLEAN(obj) (jboolean) ((obj) ? JNI_TRUE : JNI_FALSE) so that the type of the result is explicit. Unfortunately jni.h doesn't define JNI_TRUE or false with a cast to jboolean as they probably should. Mike On Mar 19 2014, at 11:36 , Sergey Bylokhov sergey.bylok...@oracle.com wrote: Thanks Anthony! Can somebody from the core-libs team take a look? On 3/17/14 10:31 PM, Anthony Petrov wrote: Personally, I'd call it to_jboolean(obj), but IS_TRUE(obj) sounds good to me too. Either way, I'm fine with the fix. -- best regards, Anthony On 3/17/2014 7:01 PM, Sergey Bylokhov wrote: Hello. This review request is for the new macro, which simplify conversion to jboolean. It will be useful for fixing parfait warnings. We have a lot of places, where we cast some type to jboolean: BOOL = retVal; return (jboolean) retVal; WARNING: Expecting value of JNI primitive type jboolean: mismatched value retVal with size 32 bits, retVal used for conversion to int8 in return +++ b/src/share/native/common/jni_util.hMon Mar 17 18:28:48 2014 +0400 @@ -277,6 +277,7 @@ #define IS_NULL(obj) ((obj) == NULL) #define JNU_IsNull(env,obj) ((obj) == NULL) +#define IS_TRUE(obj) ((obj) ? JNI_TRUE : JNI_FALSE) I am not sure about the name, probably someone have a better suggestion? The fix is for jdk9/dev. -- Best regards, Sergey. -- Best regards, Sergey.
Re: AWT Dev [9] Review request: new macro for conversion to jboolean
Hi, Well... When JNU_RETURN was added, there was a long discussion about NOT using JNU unless the JNI environment was an argument to the macro. So, the simple name is preferred. Roger On 3/19/2014 4:08 PM, Phil Race wrote: PS .. so maybe whilst you are touching this file you could do #define JNU_CHECK_NULL CHECK_NULL #define JNU_CHECK_NULL_RETURN CHECK_NULL_RETURN so we could migrate to these (clearer) ones -phil. On 3/19/2014 1:05 PM, Phil Race wrote: I think having it start with JNU_ is a good suggestion. I've been wondering over the last week or so if it would not have been better to have CHECK_NULL called JNU_CHECK_NULL to reduce collision chances and to make it clearer where it comes from .. -phil. On 3/19/2014 12:49 PM, Mike Duigou wrote: Definitely a useful macro. I too would prefer a name like TO_JBOOLEAN since it reveals the result type. Also all uppercase to identify it as a macro. If we are paranoid and want to reduce the chance of a name collision then JNU_TO_JBOOLEAN perhaps. I would also define the macro as: #define JNU_TO_JBOOLEAN(obj) (jboolean) ((obj) ? JNI_TRUE : JNI_FALSE) so that the type of the result is explicit. Unfortunately jni.h doesn't define JNI_TRUE or false with a cast to jboolean as they probably should. Mike On Mar 19 2014, at 11:36 , Sergey Bylokhov sergey.bylok...@oracle.com wrote: Thanks Anthony! Can somebody from the core-libs team take a look? On 3/17/14 10:31 PM, Anthony Petrov wrote: Personally, I'd call it to_jboolean(obj), but IS_TRUE(obj) sounds good to me too. Either way, I'm fine with the fix. -- best regards, Anthony On 3/17/2014 7:01 PM, Sergey Bylokhov wrote: Hello. This review request is for the new macro, which simplify conversion to jboolean. It will be useful for fixing parfait warnings. We have a lot of places, where we cast some type to jboolean: BOOL = retVal; return (jboolean) retVal; WARNING: Expecting value of JNI primitive type jboolean: mismatched value retVal with size 32 bits, retVal used for conversion to int8 in return +++ b/src/share/native/common/jni_util.hMon Mar 17 18:28:48 2014 +0400 @@ -277,6 +277,7 @@ #define IS_NULL(obj) ((obj) == NULL) #define JNU_IsNull(env,obj) ((obj) == NULL) +#define IS_TRUE(obj) ((obj) ? JNI_TRUE : JNI_FALSE) I am not sure about the name, probably someone have a better suggestion? The fix is for jdk9/dev. -- Best regards, Sergey. -- Best regards, Sergey.
Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64
On 3/19/2014 12:28 PM, Xueming Shen wrote: On 03/19/2014 11:37 AM, Mandy Chung wrote: https://bugs.openjdk.java.net/browse/JDK-8035807 Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8035807/webrev.00/ This patch converts the last 2 references to sun.misc.BASE64Encoder/Decoder from the jdk repo with java.util.Base64. We should also update the tests and I have filed JDK-8037873 for that. Thanks Mandy The sun.misc.BASE64En/Decoder is MIME type, so it outputs the \r\n per 76 characters during encoding, and ignores/skip \r or \n when decoding. The new Base64.getEncoder/Decoder() returns the basic Base64 coder, which it never inserts line separator when output, and throws exception for any non-base64- alphabet character, including \r and \n. The only disadvantage/incompatibility (j.u.Base64.getMimeDecoer() vs sun.misc.BASE64Decoder) of switching to j.u.Base64 MIME type en/decoder is that the Base64 Mime decoder ignores/skips any non-base64-alphabet (including \r and \n), while sun.misc.BASE64Decoder appears to simply use the init value -1 for any non-base64-alphabet character for decoding. I'm not familiar with the use scenario of ldap's Obj class, so I'm not sure if it matters (if it ever outputs/inputs 76 character data, or even it does,if the difference matters). Thanks Sherman. I believe both cases don't have the 76 characters constraint although it uses MIME type encoder/decoder previously unless Vinnie and Alan say otherwise. Btw, except getMimeEncoder(int ...) all other Base64.getXXXEn/Decoder() returns singleton, so the de/encoder cache might not be necessary. Thanks. It calls encode/decode in a loop and so I think keeping a local variable to avoid getting it multiple time seems reasonable. Mandy
Re: RFR [6943190] TEST_BUG: java/lang/Runtime/exec/ExecWithInput.java hardcodes path to cat
On 19/03/2014 17:34, Ivan Gerasimov wrote: Here's the (final?) webrev: http://cr.openjdk.java.net/~igerasim/6943190/5/webrev/ This looks okay although I would prefer for a number of these tests to fail if the command is not found (otherwise it is will just hide issues). We can create another bug for this if you'd prefer. -Alan.
Re: Review request for 8035807: Convert use of sun.misc.BASE64Encoder/Decoder with java.util.Base64
On 19/03/2014 20:33, Mandy Chung wrote: : I believe both cases don't have the 76 characters constraint although it uses MIME type encoder/decoder previously unless Vinnie and Alan say otherwise. Sherman makes a good point and we should double check the LDAP spec to see which base64 scheme this is (I suspect it is MIME). Vinnie will likely know if it's not often from the RFC. -Alan.
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Am 19.03.2014 09:19, schrieb Ivan Gerasimov:
Thank you Ulf!
On 19.03.2014 2:12, Ulf Zibis wrote:
Am 18.03.2014 19:28, schrieb Ivan Gerasimov:
Assuming this last iteration is OK, should the next step be a CCC request?
Do you mean? :
/*
* ...
+ * It is assumed that fromIndex = toIndex, otherwise the behaviour of this method is
undefined.
* ...
- * toIndex fromIndex})
* ...
*/
protected void removeRange(int fromIndex, int toIndex) {
...
The (fromIndex toIndex) condition is checked now, so the behavior of the method is defined - it
throws an exception.
Because Martin stated some days ago, that it normally should not occur, that removeRange is invoked
with toIndex fromIndex, and checking this again unnecessarily decreases performance a little, I
was hoping, your next CCC step would be to drop this check.
-Ulf
Re: JDK 9 RFR of 6375303: Review use of caching in BigDecimal
On Mar 14, 2014, at 7:17 AM, Brian Burkhalter brian.burkhal...@oracle.com wrote: On Mar 14, 2014, at 3:39 AM, Peter Levart wrote: But in general it would be better to just use ThreadLocalRandom.current() everywhere you use rnd variable. This is precisely it's purpose - a random number generator that is never contended. The overhead of ThreadLocalRandom.current() call is hardly measurable by itself. I'll update that and re-run some of the benchmarks later. Following up on the content above and this earlier message in the thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-March/025676.html I have posted a revised patch (NB: I know lines 2897-2906 should be elsewhere) http://cr.openjdk.java.net/~bpb/6375303/webrev.01/ and updated benchmark source (using only ThreadLocalRandom.current()) http://cr.openjdk.java.net/~bpb/6375303/Bench6375303.java and updated benchmark results for three different variations http://cr.openjdk.java.net/~bpb/6375303/6375303-bench-2.html This version of toString() is from Peter and dispenses with the volatile qualifier on stringCache. At least on my system, there is no statistically significant micro-performance difference among the three versions tested, viz., baseline, toString() change only, toString() change plus other cleanup. Any comments appreciated. Thanks, Brian
Re: RFR [8014066] Mistake in documentation of ArrayList#removeRange
Am 19.03.2014 23:32, schrieb Martin Buchholz: No one is as performance obsessed as Ulf. :-D :-D :-D
RFR (JAXP): 8035577: Xerces Update: impl/xpath/regex/RangeToken.java
Hi,
This is an update from Xerces for file
impl/xpath/regex/TokenRange.java. For details, please refer to:
https://bugs.openjdk.java.net/browse/JDK-8035577.
Webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8035577/webrev/
Existing tests: JAXP SQE and unit tests passed.
Test cases added for typo fix in RangeToken.intersectRanges. Code also
updated to fix a bug where regular expression intersection returns
incorrect value when first range ends later than second range. Example
below. Test cases have been added to cover any scenarios that the code
changes affect.
new RegularExpression((?[b-d][a-r])); - returns [b-d] (Correct)
new RegularExpression((?[a-r][b-d])); - returns [b-de-r] (Incorrect)
Thanks,
David
P.S. Notes on bug fixes.
1) Line 404 removal of while loop.
This fixes a new bug where incorrect results are given when first range
ends later than second range. In the old code we got
(?[a-r][b-d]) - returns [b-de-r]
By removing the while loop, we get [b-d].
This while loop looks like a copy-paste error from subtractRanges. In
subtractRanges we need to keep the leftover portion from the first
range, but this does not apply to intersection.
2) Line 388, addition of src2 += 2;
This code change affects anything of the form (?[a-r][b-eg-j]). The
code execution is diagrammed below.
oo (src1)
o--o o--o (src2)
For the first match we get
oo (src1)
o--o (src2)
Next we want to run src2+=2 to get the second pair of endpoints (since
the first two endpoints are already used). Notice how src1begin has
been updated to this.ranges[src1] = src2end+1, which is directly from
the code.
o--o (src1)
o--o (src2)
The src2+=2 statement was left out of the old code, and is added in this
webrev. If we leave out the src2+=2 at line 388, on the next iteration
of the large while loop we will reach case } else if (src2end
src1begin) { which also executes src2+=2. This means the correct
final result is generated, but on a later loop. We want to add the new
code because it's better to have all associated variable updated in the
sameloop. In addition, all the other conditions have similar src1 or
src2 updates.
Re: JDK 9 RFR of 6375303: Review use of caching in BigDecimal
On 03/19/2014 11:01 PM, Brian Burkhalter wrote:
On Mar 14, 2014, at 7:17 AM, Brian Burkhalter
brian.burkhal...@oracle.com mailto:brian.burkhal...@oracle.com wrote:
On Mar 14, 2014, at 3:39 AM, Peter Levart wrote:
But in general it would be better to just use
ThreadLocalRandom.current() everywhere you use rnd variable.
This is precisely it's purpose - a random number generator that is
never contended. The overhead of ThreadLocalRandom.current() call is
hardly measurable by itself.
I'll update that and re-run some of the benchmarks later.
Following up on the content above and this earlier message in the thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-March/025676.html
I have posted a revised patch (NB: I know lines 2897-2906 should be
elsewhere)
http://cr.openjdk.java.net/~bpb/6375303/webrev.01/
http://cr.openjdk.java.net/%7Ebpb/6375303/webrev.01/
and updated benchmark source (using only ThreadLocalRandom.current())
http://cr.openjdk.java.net/~bpb/6375303/Bench6375303.java
http://cr.openjdk.java.net/%7Ebpb/6375303/Bench6375303.java
and updated benchmark results for three different variations
http://cr.openjdk.java.net/~bpb/6375303/6375303-bench-2.html
http://cr.openjdk.java.net/%7Ebpb/6375303/6375303-bench-2.html
This version of toString() is from Peter and dispenses with the
volatile qualifier on stringCache. At least on my system, there is no
statistically significant micro-performance difference among the three
versions tested, viz., baseline, toString() change only, toString()
change plus other cleanup.
Any comments appreciated.
Thanks,
Brian
Hi Brian,
Here's my promised run of your latest webrev and microbenchmark on ARM
platform (Raspberry Pi) with just released JDK 8 for ARM (-client
compiler, since -server does not work on Raspberry Pi):
org.openjdk.jmh.Main parameters: .* -i 10 -r 5 -wi 5 -w 1 -f 1 -t 1
--- Baseline, 1-thread ---
Benchmark Mode Samples Mean Mean error
Units
o.s.Bench6375303.testFirstToString avgt10 330618.266 2211.637
ns/op
o.s.Bench6375303.testToString avgt10 80.5460.134
ns/op
--- Proposed webrev, 1-thread ---
Benchmark Mode Samples Mean Mean error
Units
o.s.Bench6375303.testFirstToString avgt10 326588.284 1714.892
ns/op
o.s.Bench6375303.testToString avgt10 102.5820.295
ns/op
--- Previous variant with volatile stringCache field, 1-thread ---
Benchmark Mode Samples Mean Mean error
Units
o.s.Bench6375303.testFirstToString avgt10 328795.783 2508.173
ns/op
o.s.Bench6375303.testToString avgt10 105.7410.316
ns/op
So both variants seem to be more or less the same but slower than baseline.
Why would they be slower? Answer: they have more bytecodes.
If I run with following JVM options: -XX:+UnlockDiagnosticVMOptions -XX:MaxInlineSize=100
(and only the testToString benchmark), I get:
--- Baseline, 1-thread ---
Benchmark Mode Samples Mean Mean error
Units
o.s.Bench6375303.testToString avgt10 80.8390.742
ns/op
--- Proposed webrev, 1-thread ---
Benchmark Mode Samples Mean Mean error
Units
o.s.Bench6375303.testToString avgt10 80.8510.771
ns/op
--- Previous variant with volatile stringCache field, 1-thread ---
Benchmark Mode Samples Mean Mean error
Units
o.s.Bench6375303.testToString avgt10 80.8340.749
ns/op
So the solution is to reduce number of bytecodes in toString(). For example,
the following:
public String toString() {
String sc = stringCache;
if (sc == null) {
sc = toStringSlow();
}
return sc;
}
private String toStringSlow() {
String sc = (String) U.getObjectVolatile(this, STRING_CACHE_OFFSET);
if (sc == null) {
sc = layoutChars(true);
if (!U.compareAndSwapObject(this, STRING_CACHE_OFFSET, null, sc)) {
sc = (String) U.getObjectVolatile(this, STRING_CACHE_OFFSET);
}
}
return sc;
}
...gives the good results even without special JVM options:
Benchmark Mode Samples Mean Mean error
Units
o.s.Bench6375303.testToString avgt10 80.9250.313
ns/op
Regards, Peter
Re: JDK 9 RFR of 6375303: Review use of caching in BigDecimal
I On Mar 19 2014, at 15:01 , Brian Burkhalter brian.burkhal...@oracle.com wrote: On Mar 14, 2014, at 7:17 AM, Brian Burkhalter brian.burkhal...@oracle.com wrote: On Mar 14, 2014, at 3:39 AM, Peter Levart wrote: But in general it would be better to just use ThreadLocalRandom.current() everywhere you use rnd variable. This is precisely it's purpose - a random number generator that is never contended. The overhead of ThreadLocalRandom.current() call is hardly measurable by itself. I'll update that and re-run some of the benchmarks later. Following up on the content above and this earlier message in the thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-March/025676.html I have posted a revised patch (NB: I know lines 2897-2906 should be elsewhere) http://cr.openjdk.java.net/~bpb/6375303/webrev.01/ and updated benchmark source (using only ThreadLocalRandom.current()) http://cr.openjdk.java.net/~bpb/6375303/Bench6375303.java and updated benchmark results for three different variations http://cr.openjdk.java.net/~bpb/6375303/6375303-bench-2.html This version of toString() is from Peter and dispenses with the volatile qualifier on stringCache. At least on my system, there is no statistically significant micro-performance difference among the three versions tested, viz., baseline, toString() change only, toString() change plus other cleanup. Since the Unsafe.getObjectVolatile() allows use of volatile semantics without having to declare the field volatile I think this is a better idiom than what I had previously suggested. Hopefully this is a pattern we can use elsewhere. Are the java.util.concurrent.atomic imports still needed? I have not reviewed the other changes. Mike
Re: JDK 9 RFR of 6375303: Review use of caching in BigDecimal
On Mar 19, 2014, at 4:32 PM, Mike Duigou mike.dui...@oracle.com wrote: Since the Unsafe.getObjectVolatile() allows use of volatile semantics without having to declare the field volatile I think this is a better idiom than what I had previously suggested. Hopefully this is a pattern we can use elsewhere. Are the java.util.concurrent.atomic imports still needed? No they are not. I can remove them (and move the code at lines 2897-2906 to following coding conventions) if this is eventually approved. I have not reviewed the other changes. Aside from toString() they are mostly straightforward cleanup. Thanks, Brian
Re: JDK 9 RFR of 6375303: Review use of caching in BigDecimal
Hi Peter,
On Mar 19, 2014, at 4:32 PM, Peter Levart peter.lev...@gmail.com wrote:
So the solution is to reduce number of bytecodes in toString(). For
example, the following:
public String toString() {
String sc = stringCache;
if (sc == null) {
sc = toStringSlow();
}
return sc;
}
private String toStringSlow() {
String sc = (String) U.getObjectVolatile(this, STRING_CACHE_OFFSET);
if (sc == null) {
sc = layoutChars(true);
if (!U.compareAndSwapObject(this, STRING_CACHE_OFFSET, null, sc))
{
sc = (String) U.getObjectVolatile(this, STRING_CACHE_OFFSET);
}
}
return sc;
}
...gives the good results even without special JVM options:
Benchmark Mode Samples Mean Mean error
Units
o.s.Bench6375303.testToString avgt10 80.9250.313
ns/op
That’s great! I can re-try that version on my system for comparison.
Thanks,
Brian
Re: AWT Dev [9] Review request: new macro for conversion to jboolean
Thanks. So if nobody objects, the final version will be: #define IS_NULL(obj) ((obj) == NULL) #define JNU_IsNull(env,obj) ((obj) == NULL) +#define TO_JBOOLEAN(obj) (jboolean) ((obj) ? JNI_TRUE : JNI_FALSE) On 3/20/14 12:07 AM, roger riggs wrote: Hi, Well... When JNU_RETURN was added, there was a long discussion about NOT using JNU unless the JNI environment was an argument to the macro. So, the simple name is preferred. Roger On 3/19/2014 4:08 PM, Phil Race wrote: PS .. so maybe whilst you are touching this file you could do #define JNU_CHECK_NULL CHECK_NULL #define JNU_CHECK_NULL_RETURN CHECK_NULL_RETURN so we could migrate to these (clearer) ones -phil. On 3/19/2014 1:05 PM, Phil Race wrote: I think having it start with JNU_ is a good suggestion. I've been wondering over the last week or so if it would not have been better to have CHECK_NULL called JNU_CHECK_NULL to reduce collision chances and to make it clearer where it comes from .. -phil. On 3/19/2014 12:49 PM, Mike Duigou wrote: Definitely a useful macro. I too would prefer a name like TO_JBOOLEAN since it reveals the result type. Also all uppercase to identify it as a macro. If we are paranoid and want to reduce the chance of a name collision then JNU_TO_JBOOLEAN perhaps. I would also define the macro as: #define JNU_TO_JBOOLEAN(obj) (jboolean) ((obj) ? JNI_TRUE : JNI_FALSE) so that the type of the result is explicit. Unfortunately jni.h doesn't define JNI_TRUE or false with a cast to jboolean as they probably should. Mike On Mar 19 2014, at 11:36 , Sergey Bylokhov sergey.bylok...@oracle.com wrote: Thanks Anthony! Can somebody from the core-libs team take a look? On 3/17/14 10:31 PM, Anthony Petrov wrote: Personally, I'd call it to_jboolean(obj), but IS_TRUE(obj) sounds good to me too. Either way, I'm fine with the fix. -- best regards, Anthony On 3/17/2014 7:01 PM, Sergey Bylokhov wrote: Hello. This review request is for the new macro, which simplify conversion to jboolean. It will be useful for fixing parfait warnings. We have a lot of places, where we cast some type to jboolean: BOOL = retVal; return (jboolean) retVal; WARNING: Expecting value of JNI primitive type jboolean: mismatched value retVal with size 32 bits, retVal used for conversion to int8 in return +++ b/src/share/native/common/jni_util.hMon Mar 17 18:28:48 2014 +0400 @@ -277,6 +277,7 @@ #define IS_NULL(obj) ((obj) == NULL) #define JNU_IsNull(env,obj) ((obj) == NULL) +#define IS_TRUE(obj) ((obj) ? JNI_TRUE : JNI_FALSE) I am not sure about the name, probably someone have a better suggestion? The fix is for jdk9/dev. -- Best regards, Sergey. -- Best regards, Sergey. -- Best regards, Sergey.
Re: JDK 9 RFR of 6375303: Review use of caching in BigDecimal
On Mar 19, 2014, at 4:41 PM, Brian Burkhalter brian.burkhal...@oracle.com wrote: Benchmark Mode Samples Mean Mean error Units o.s.Bench6375303.testToString avgt10 80.9250.313 ns/op That’s great! I can re-try that version on my system for comparison. Here are the results on my system: http://cr.openjdk.java.net/~bpb/6375303/6375303-bench-3.html Thanks, Brian
Re: AWT Dev [9] Review request: new macro for conversion to jboolean
Hi Sergey, Please give this a day to stew. I'd like some time to consider other names before agreeing. (Not that my agreement is necessary or sufficient). Thanks, Roger On 3/19/14 7:43 PM, Sergey Bylokhov wrote: Thanks. So if nobody objects, the final version will be: #define IS_NULL(obj) ((obj) == NULL) #define JNU_IsNull(env,obj) ((obj) == NULL) +#define TO_JBOOLEAN(obj) (jboolean) ((obj) ? JNI_TRUE : JNI_FALSE) On 3/20/14 12:07 AM, roger riggs wrote: Hi, Well... When JNU_RETURN was added, there was a long discussion about NOT using JNU unless the JNI environment was an argument to the macro. So, the simple name is preferred. Roger On 3/19/2014 4:08 PM, Phil Race wrote: PS .. so maybe whilst you are touching this file you could do #define JNU_CHECK_NULL CHECK_NULL #define JNU_CHECK_NULL_RETURN CHECK_NULL_RETURN so we could migrate to these (clearer) ones -phil. On 3/19/2014 1:05 PM, Phil Race wrote: I think having it start with JNU_ is a good suggestion. I've been wondering over the last week or so if it would not have been better to have CHECK_NULL called JNU_CHECK_NULL to reduce collision chances and to make it clearer where it comes from .. -phil. On 3/19/2014 12:49 PM, Mike Duigou wrote: Definitely a useful macro. I too would prefer a name like TO_JBOOLEAN since it reveals the result type. Also all uppercase to identify it as a macro. If we are paranoid and want to reduce the chance of a name collision then JNU_TO_JBOOLEAN perhaps. I would also define the macro as: #define JNU_TO_JBOOLEAN(obj) (jboolean) ((obj) ? JNI_TRUE : JNI_FALSE) so that the type of the result is explicit. Unfortunately jni.h doesn't define JNI_TRUE or false with a cast to jboolean as they probably should. Mike On Mar 19 2014, at 11:36 , Sergey Bylokhov sergey.bylok...@oracle.com wrote: Thanks Anthony! Can somebody from the core-libs team take a look? On 3/17/14 10:31 PM, Anthony Petrov wrote: Personally, I'd call it to_jboolean(obj), but IS_TRUE(obj) sounds good to me too. Either way, I'm fine with the fix. -- best regards, Anthony On 3/17/2014 7:01 PM, Sergey Bylokhov wrote: Hello. This review request is for the new macro, which simplify conversion to jboolean. It will be useful for fixing parfait warnings. We have a lot of places, where we cast some type to jboolean: BOOL = retVal; return (jboolean) retVal; WARNING: Expecting value of JNI primitive type jboolean: mismatched value retVal with size 32 bits, retVal used for conversion to int8 in return +++ b/src/share/native/common/jni_util.hMon Mar 17 18:28:48 2014 +0400 @@ -277,6 +277,7 @@ #define IS_NULL(obj) ((obj) == NULL) #define JNU_IsNull(env,obj) ((obj) == NULL) +#define IS_TRUE(obj) ((obj) ? JNI_TRUE : JNI_FALSE) I am not sure about the name, probably someone have a better suggestion? The fix is for jdk9/dev. -- Best regards, Sergey. -- Best regards, Sergey.
