Re: Review/comment needed for the new public java.util.Base64 class
Am 24.10.2012 04:56, schrieb Xueming Shen:
Thanks for the review. I hope I addressed most of the comments in the
updated webrev at
http://cr.openjdk.java.net/~sherman/4235519/webrev
Hi Sherman,
my additional comments:
I'm confused about the order of xxcode() and Xxcoder.
In other places e.g. charsets, we have de... before en..., which is also true
alphabetical
1026 private Base64() {}
1027 private static final int MIMELINEMAX = 76;
1028 private static final byte[] crlf = new byte[] {'\r', '\n'};
- difficult to find, please move (more) to the beginning, at least not in between inner class
definitions.
- I more would like CRLF than crlf
118 * @param lineLengththe length of each output line (rounded down
119 * to nearest multiple of 4).
This could be interpreted as: the parameter must be rounded before, so maybe
better:
118 * @param lineLengththe length of each output line (if not a
multiple of 4,
119 * it will be rounded down accordingly).
126 public static Encoder getEncoder(int lineLength, byte[] lineSeparator)
{
127 Objects.requireNonNull(lineSeparator);
128 return new Encoder(false, lineSeparator, lineLength / 4 * 4);
129 }
- What (should) happen(s), if lineSeparator.length == 0 ?
- Isn't one of these little faster, or at least more clear? :
lineLength -= lineLength % 4
lineLength 0xFFFC
lineLength 2 2
Broken indentation (why at all, compared to lines 213..215?):
208 private final byte[] newline;
209 private final intlinemax;
210 private final boolean isURL;
Unconventional indentation and even broken in lines 223..224 (maybe more?):
247 * @param src the byte array to encode
248 * @param dst the output byte array
249 * @returnThe number of bytes written to the output byte
array
250 *
251 * @throwsIllegalArgumentException if {@code dst} does not
have
252 *enough space for encoding all input bytes.
More conventional:
247 * @param src the byte array to encode
248 * @param dst the output byte array
249 * @return The number of bytes written to the output byte array
250 *
251 * @throws IllegalArgumentException if {@code dst} does not have
252 * enough space for encoding all input bytes.
603 static {
604 Arrays.fill(fromBase64, -1);
605 for (int i = 0; i Encoder.toBase64.length; i++)
606 fromBase64[Encoder.toBase64[i]] = i;
607 fromBase64['='] = -2;
608 }
This causes the inner Encoder class to be loaded, even if not needed. So maybe move the toBase64
table to the outer class.
Have you compared performance with fromBase64 as byte[] (including local 'b' in
decode() as byte)?
Retrieving the bytes by b = base64[x] then could be done without address-shift and smaller/faster
LoadB operations could be used by JIT. At least the footprint of the table would be smaller.
Missing spaces:
805 return new DecInputStream(is, isURL?fromBase64URL:fromBase64,
isMIME);
Why at all you repeat this code many times? Why not? :
631 this.base64 = isURL ? fromBase64URL : fromBase64;
Also it would at least be helpful for readers to define final, e.g.:
809 final int[] base64 = isURL ? fromBase64URL : fromBase64;
Why you mix the algorithms to check the padding? :
824 if (b == -2) { // padding byte
890 if (b == '=') {
-Ulf
Review request for 8001536
Hi, This is a request for review of http://cr.openjdk.java.net/~lancea/8001536/webrev.00/. This adds read/writeObject as well as clone methods to SerialXLob classes. All SQE tests passed, 1 failure in the RowSet JCK/TCK tests due to a bug in the test that the TCK team is aware of and will address. JDBC Unit tests all pass . Best Lance 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: Review/comment needed for the new public java.util.Base64 class
Oops: lineLength 2 2 I meant:: lineLength 2 2 -Ulf
hg: jdk8/tl/langtools: 8001929: fix doclint errors in langtools doc comments
Changeset: 23fe1a96bc0f Author:jjg Date: 2012-10-30 10:15 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/23fe1a96bc0f 8001929: fix doclint errors in langtools doc comments Reviewed-by: darcy ! src/share/classes/com/sun/tools/doclets/formats/html/HtmlDocletWriter.java ! src/share/classes/com/sun/tools/doclets/formats/html/SplitIndexWriter.java ! src/share/classes/com/sun/tools/doclets/formats/html/markup/HtmlDocWriter.java ! src/share/classes/com/sun/tools/doclets/formats/html/markup/HtmlWriter.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/util/DocPath.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/util/SourcePath.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/util/Util.java ! src/share/classes/com/sun/tools/javac/code/TypeTag.java ! src/share/classes/com/sun/tools/javac/comp/Attr.java
Re: Review request for 8001536
On 10/30/2012 05:25 PM, Lance Andersen - Oracle wrote: Hi, This is a request for review of http://cr.openjdk.java.net/~lancea/8001536/webrev.00/. This adds read/writeObject as well as clone methods to SerialXLob classes. All SQE tests passed, 1 failure in the RowSet JCK/TCK tests due to a bug in the test that the TCK team is aware of and will address. JDBC Unit tests all pass . Hi Lance. In SerialBlob and in SerialClob test (obj == null) is not necessary in equals, null instanceof X is always false. in hashCode, Objects.hash() allocate an array to pass arguments to Arrays.hashCode() and box primitive values to Object. while this method is really convenient to use, each calls will allocate an array and box the two values, the overhead seems to high here. This code should be equivalent: return ((31 +Arrays.hashCode(buf)) * 31 +len) * 31 + origLen; in clone, sb should not be initialized to null and the catch should be: throw new InternalError(e), this is the standard code you can see in clone. in readObject, the test (buf.length != len) can be done before decoding the blob. in writeObject, you set blob twice, which is weird, also I think that if blob is not Serializable, the code should throw an exception, so you should not use instanceof and let s.writeFields() to throw NotSerializable exception. cheers, Rémi Best Lance 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: Review request for 8001536
Hi all, please add the bug summary to the subject line. Bug 8001536 is not publicly visible :-( -Ulf Am 30.10.2012 17:25, schrieb Lance Andersen - Oracle: Hi, This is a request for review of http://cr.openjdk.java.net/~lancea/8001536/webrev.00/. This adds read/writeObject as well as clone methods to SerialXLob classes. All SQE tests passed, 1 failure in the RowSet JCK/TCK tests due to a bug in the test that the TCK team is aware of and will address. JDBC Unit tests all pass . Best Lance 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: Review request for 8001536
Hi Ulf, The bug is described below, it is just adding the read/writeObject and clone methods. Best Lance On Oct 30, 2012, at 2:18 PM, Ulf Zibis wrote: Hi all, please add the bug summary to the subject line. Bug 8001536 is not publicly visible :-( -Ulf Am 30.10.2012 17:25, schrieb Lance Andersen - Oracle: Hi, This is a request for review of http://cr.openjdk.java.net/~lancea/8001536/webrev.00/. This adds read/writeObject as well as clone methods to SerialXLob classes. All SQE tests passed, 1 failure in the RowSet JCK/TCK tests due to a bug in the test that the TCK team is aware of and will address. JDBC Unit tests all pass . Best Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: RFR: 6594697 - varargs message and Throwable methods for java.util.Logger
On 30/10/2012 14:21, Jim Gish wrote: I was one the fence with the parameter ordering and would like additional feedback on this point. I started off as you suggested, but didn't like the fact that the params were separated from the msg by the Throwable. I could go either way, but would like to hear from others on this point. Thanks, Jim I think it make sense to keep the message and its parameters together, and since the parameters have to be at the end then it does mean the Throwable comes before the message. It does mean that there is a bit of inconsistency in the ordering but it might be a bit odd to put the Throwable between the message and its parameters. I think the changes to use isLoggable should be separated out into their own issue. -Alan
Re: Review/comment needed for the new public java.util.Base64 class
Hi Sherman, thanks for your details. Has this discussion been on the list, and I have missed it? I see a problem with hiding the singleton choice: Developers might tend to repetitively invoke public static Encoder getEncoder(int lineLength, byte[] lineSeparator) instead reusing the the same instance. (Note: It should not harm, to move the Objects.requireNonNull(lineSeparator); to the general private constructor.) In case of a public constant Encoder.RFC4648 ... developer would be aware about the re-usability of the encoder. IMHO, the get...() methods are just waste of source lines and bytecode footprint. But in contrast to the v4 I like the inner class approach: Base64.De/Encoder. Little nit: In lines 89,100,127,128,138,149,159 the indentation is 5 instead 4. -Ulf Am 30.10.2012 02:51, schrieb Xueming Shen: Ulf, my apology. Some how I missed your email. We tried various prototypes for this simple utility class. See http://cr.openjdk.java.net/~sherman/base64/ The v4 might be close to the static constant approach you suggested. While It's hard to draw a clear line on which one is better, we concluded that the proposed approach provides the best balance among usability, readability and extensibility. For example, the get approach allows us to hide the singleton choice to the implementation. It provides a consistent interface fixed basic/url/mime type en/decoder and the configurable floating length/linefeed encoder. -Sherman On 10/29/12 11:15 AM, Ulf Zibis wrote: Hi Sherman, can you give me a short answer please? -Ulf Am 23.10.2012 16:57, schrieb Ulf Zibis: Am 23.10.2012 15:04, schrieb Alan Bateman: I'm not sure that getUrlEncoder is the most suitable name to get a base64url encoder. The reason is that the method name makes it sound like it returns a URLEncoder or or at least an encoder for HTML forms. While more verbose, getBase64UrlEncoder is clear that it returns a base64url encoder. I'm wondering, why there are those get... methods at all. Alternatively you could make the appropriate constructors and predifined static variants public. So one only should use: Base64.Encoder encoder = new Base64.Encoder(...); Base64.Encoder urlEncoder = Base64.Encoder.RFC4648_URLSAFE; No need for those looong method names. -Ulf
Re: Review request for 8001536
Thanks Lance. But having the subject of the request in clear text in the list view of the email client would be a great help. -Ulf Am 30.10.2012 19:28, schrieb Lance Andersen - Oracle: Hi Ulf, The bug is described below, it is just adding the read/writeObject and clone methods. Best Lance On Oct 30, 2012, at 2:18 PM, Ulf Zibis wrote: Hi all, please add the bug summary to the subject line. Bug 8001536 is not publicly visible :-( -Ulf Am 30.10.2012 17:25, schrieb Lance Andersen - Oracle: Hi, This is a request for review of http://cr.openjdk.java.net/~lancea/8001536/webrev.00/. This adds read/writeObject as well as clone methods to SerialXLob classes. All SQE tests passed, 1 failure in the RowSet JCK/TCK tests due to a bug in the test that the TCK team is aware of and will address. JDBC Unit tests all pass . Best Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com 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: Review request for 8001536
Hi Remi, Thank you for the feedback On Oct 30, 2012, at 2:05 PM, Remi Forax wrote: On 10/30/2012 05:25 PM, Lance Andersen - Oracle wrote: Hi, This is a request for review of http://cr.openjdk.java.net/~lancea/8001536/webrev.00/. This adds read/writeObject as well as clone methods to SerialXLob classes. All SQE tests passed, 1 failure in the RowSet JCK/TCK tests due to a bug in the test that the TCK team is aware of and will address. JDBC Unit tests all pass . Hi Lance. In SerialBlob and in SerialClob test (obj == null) is not necessary in equals, null instanceof X is always false. OK, thanks for the suggestion, I will make this change in hashCode, Objects.hash() allocate an array to pass arguments to Arrays.hashCode() and box primitive values to Object. while this method is really convenient to use, each calls will allocate an array and box the two values, the overhead seems to high here. This code should be equivalent: return ((31 +Arrays.hashCode(buf)) * 31 +len) * 31 + origLen; I can simplify hashCode to the what you have above, I liked the convenience method which is why I was using it. But happy to change it in clone, sb should not be initialized to null I think it is OK as I have it as HashMap does it similar to what I have done vs ArrayList which follows your suggestion. Do we have a preferred practice or is this just a style choice? and the catch should be: throw new InternalError(e), Given I am providing clone(), I did not see a reason to provide InternalError(). I have no strong preference but some java classes do and others do not (HashMap for example), so what is our preferred standard? this is the standard code you can see in clone. in readObject, the test (buf.length != len) can be done before decoding the blob. True, I can move it up. in writeObject, you set blob twice, which is weird, my bad, I forgot to remove that. also I think that if blob is not Serializable, the code should throw an exception, so you should not use instanceof and let s.writeFields() to throw NotSerializable exception. This is intentional. A Blob or Clob will not be serializable as its properties are unique to the database and it is created from an active Connection object. In the event someone actually tried to serialize this, I do not want it to fail just because someone passed in a LOB to instantiate this beast (note these methods should never have been created this way but this is way before my time). In the unlikely event someone created their own wrapped Blob/Clob (which I cannot see why anyone would do), I am allowing for both for backwards compatibility. Best Lance cheers, Rémi Best Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com 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: Review request for 8001536
Point taken Ulf, thank you for the feedback and will follow your suggestion going forward , which I typically do, but I think I am still feeling the effects of being offline due to hurricane sandy :-( Best Lance On Oct 30, 2012, at 2:50 PM, Ulf Zibis wrote: Thanks Lance. But having the subject of the request in clear text in the list view of the email client would be a great help. -Ulf Am 30.10.2012 19:28, schrieb Lance Andersen - Oracle: Hi Ulf, The bug is described below, it is just adding the read/writeObject and clone methods. Best Lance On Oct 30, 2012, at 2:18 PM, Ulf Zibis wrote: Hi all, please add the bug summary to the subject line. Bug 8001536 is not publicly visible :-( -Ulf Am 30.10.2012 17:25, schrieb Lance Andersen - Oracle: Hi, This is a request for review of http://cr.openjdk.java.net/~lancea/8001536/webrev.00/. This adds read/writeObject as well as clone methods to SerialXLob classes. All SQE tests passed, 1 failure in the RowSet JCK/TCK tests due to a bug in the test that the TCK team is aware of and will address. JDBC Unit tests all pass . Best Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com 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: Review request for 8001536
On 30/10/2012 18:05, Remi Forax wrote: in writeObject, you set blob twice, which is weird, also I think that if blob is not Serializable, the code should throw an exception, so you should not use instanceof and let s.writeFields() to throw NotSerializable exception. Yes, that is odd. I think the issue here is that serialized form should never have included the blob in the first place. -Alan
Re: RFR: 6594697 - varargs message and Throwable methods for java.util.Logger
I've prepared a new webrev retaining the parameter ordering, but removing the isLoggable() refactoring for now. http://cr.openjdk.java.net/~jgish/Bug6594697-AddLogThrowable/ http://cr.openjdk.java.net/%7Ejgish/Bug6594697-AddLogThrowable/ Thanks, Jim If this looks o.k. I'll submit to CCC for approval. On 10/30/2012 02:30 PM, Alan Bateman wrote: On 30/10/2012 14:21, Jim Gish wrote: I was one the fence with the parameter ordering and would like additional feedback on this point. I started off as you suggested, but didn't like the fact that the params were separated from the msg by the Throwable. I could go either way, but would like to hear from others on this point. Thanks, Jim I think it make sense to keep the message and its parameters together, and since the parameters have to be at the end then it does mean the Throwable comes before the message. It does mean that there is a bit of inconsistency in the ordering but it might be a bit odd to put the Throwable between the message and its parameters. I think the changes to use isLoggable should be separated out into their own issue. -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: Review/comment needed for the new public java.util.Base64 class
Hi Ulf, thanks for the comments.
Webrev has been updated to address most of your comments.
http://cr.openjdk.java.net/~sherman/4235519/webrev
(1) all @param and @return tags have been normalized.
(2) private constructor BAse64() {} has been moved up.
(3) MIMELINEMAX and CRLF have been moved into encoder.
(4) - lineLength 22;
Your questions:
I'm confused about the order of xxcode() and Xxcoder.
In other places e.g. charsets, we have de... before en..., which is
also true alphabetical
should not be an issue. javadoc output should be in alphabetic order. If
it is really
concerns you, I can do a copy/paste:-)
- What (should) happen(s), if lineSeparator.length == 0 ?
do nothing. expected?
- Isn't one of these little faster, or at least more clear? :
lineLength -= lineLength % 4
lineLength 0xFFFC
lineLength 2 2
603 static {
604 Arrays.fill(fromBase64, -1);
605 for (int i = 0; i Encoder.toBase64.length; i++)
606 fromBase64[Encoder.toBase64[i]] = i;
607 fromBase64['='] = -2;
608 }
This causes the inner Encoder class to be loaded, even if not needed.
So maybe move the toBase64 table to the outer class.
Have you compared performance with fromBase64 as byte[] (including
local 'b' in decode() as byte)?
understood. but it appears the hotspot likes it the constant/fixed
length lookup
table is inside encoder. Same as you suggestion in your previous email
to use
String in source and expand it during runtime. It saves about 500 bytes
but slows
the server vm. Those repeating lines of isURL? is also supposed
to be a
performance tweak to eliminate the array boundary check in loop.
Why you mix the algorithms to check the padding? :
824 if (b == -2) { // padding byte
890 if (b == '=') {
It is supposed reduce one if check for normal base64 character inside the
loop. I'm not that sure it really helps though.
-Sherman
