Re: Review request 8002212 - adding read/writeObject to additional SerialXXX classes -- Updated
I revised the webrev, http://cr.openjdk.java.net/~lancea/8002212/webrev.01,
taking into account the vast majority of Remi's suggestions.
I also added SerialStruct to the webrev.
Have a great weekend.
Best
Lance
On Nov 2, 2012, at 7:42 PM, Remi Forax wrote:
On 11/02/2012 11:57 PM, Lance Andersen - Oracle wrote:
This is similar to 8001536, just additional classes.
This adds read/writeObject, equals, clone methods to additional SerialXXX
classes
SQE, JCK and JDBC Unit tests all pass.
The webrev can be viewed at
http://cr.openjdk.java.net/~lancea/8002212/webrev.00
Hi Lance,
in SerialArray.equals(), I prefer
return baseType == sa.baseType
baseTypeName.equals(sa.baseTypeName))
Arrays.equals(elements, sa.elements);
instead of
if(baseType == sa.baseType baseTypeName.equals(sa.baseTypeName)) {
return Arrays.equals(elements, sa.elements);
}
In SerialDataLink, do you really need readObject/writeObject given
that you call the default implementations.
in SerialJavaObject, in equals, you should declare a local variable like in
SerialDataLink.equals,
even if the local varialble is used once, it's more readable.
Also like in SerialArray.equals, you can do a return directly instead of
if(...) return true.
in clone(), you can use the diamond syntax,
sjo.chain = new Vector(chain);
in setWarning(), you can use the diamond syntax as the original source does.
and in readObject, you can use the diamond syntax too.
In readObject, you forget to throw an exception if there are some static
fields
in getClass().getFields() as the constructor does
(this code can be moved in a private static method).
Also, you should add a comment that because you call getClass() on obj,
there is an implicit null check.
This can be fixed as a separated bug or not but the code of
method SerialJavaObject.getField() is weird, the code checks if fields can be
null,
but fields is never null. Also, cloning the field array is perhaps a better
idea
if the reflection implementation doesn't cache the array of fields.
In SerialRef.equals, again, if(...) return should be transformed into return
...
Best
Lance
cheers,
Rémi
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 8002212 - adding read/writeObject to additional SerialXXX classes
On Nov 3, 2012, at 11:14 AM, Remi Forax wrote: On 11/03/2012 01:46 AM, Lance Andersen - Oracle wrote: Hi Remi, [...] In SerialDataLink, do you really need readObject/writeObject given that you call the default implementations. I thought about that but had decided to add them for consistency The serialization implementation needs to create metadata associated with readObject/writeObject, so if we can avoid to implement them, we reduce the runtime memory footprint a little. I would prefer to have a comment saying that default serialization is Ok here. OK, I will just comment the methods out and add a comment as you suggest in SerialJavaObject, in equals, you should declare a local variable like in SerialDataLink.equals, even if the local varialble is used once, it's more readable. OK Also like in SerialArray.equals, you can do a return directly instead of if(...) return true. in clone(), you can use the diamond syntax, sjo.chain = new Vector(chain); Yeah, long story, but I forgot to reset to diamond syntax (will tell you over a beer sometime :-) ) sure, now or you have to visit Paris or I have to go to NY :) Or Boston, where I am based or perhaps a JavaOne ;-) in setWarning(), you can use the diamond syntax as the original source does. and in readObject, you can use the diamond syntax too. OK In readObject, you forget to throw an exception if there are some static fields in getClass().getFields() as the constructor does (this code can be moved in a private static method). I thought about that, but figured since it was already through that check when the object was created, I did not think it required repeating in the readObject method A serialization stream can be forged to encode a SerialJavaObject that references an object that have a static field without creating a SerialJavaObject in memory. True and there are tests in the test suite that basically do that. Anyways, I added that check in the revised webrev that I pushed earlier. Also, you should add a comment that because you call getClass() on obj, there is an implicit null check. Would it be better to put an null check in explicitly? As you prefer :) [...] Thank you again, will send an update webrev over the weekend 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
Re: Review request 8002212 - adding read/writeObject to additional SerialXXX classes -- Updated
On 11/03/2012 03:11 PM, Lance Andersen - Oracle wrote:
I revised the webrev, http://cr.openjdk.java.net/~lancea/8002212/webrev.01,
taking into account the vast majority of Remi's suggestions.
in SerialJavaObject, hasStaticFields doesn't work, the original code
doesn't work because
it only check for fields that are declared static but not for fields
that are by example public static.
private static boolean hasStaticFields(Field[] fields) {
for (Field field : fields) {
if ( Modifier.isStatic(field.getModifiers())) {
return true;
}
}
return false;
}
This may cause compatibility issue because despite the specification,
the original code
will let objects that have a static field to be serialized.
Also, in readObject, if obj is null, the code should throw an
IOException because
it's not possible to create a SerialJavaObject with null has parameter
(because obj.getClass()
that implictly checks null in the constructor).
All other classes are Ok for me.
I also added SerialStruct to the webrev.
SerialStruct is Ok for me.
Have a great weekend.
Have a nice weekend too.
Best
Lance
cheers,
Rémi
Re: Review request 8002212 - adding read/writeObject to additional SerialXXX classes -- Updated
On Nov 3, 2012, at 11:34 AM, Remi Forax wrote:
On 11/03/2012 03:11 PM, Lance Andersen - Oracle wrote:
I revised the webrev, http://cr.openjdk.java.net/~lancea/8002212/webrev.01,
taking into account the vast majority of Remi's suggestions.
in SerialJavaObject, hasStaticFields doesn't work, the original code doesn't
work because
it only check for fields that are declared static but not for fields that are
by example public static.
private static boolean hasStaticFields(Field[] fields) {
for (Field field : fields) {
if ( Modifier.isStatic(field.getModifiers())) {
return true;
}
}
return false;
}
This may cause compatibility issue because despite the specification, the
original code
will let objects that have a static field to be serialized.
I cannot make the change above as it breaks too many tests and I would prefer
to go with the less is more scenario. As I think I mentioned before, I do not
think the original authors really thought through these classes and thankfully
they are not used much, if at all.
Also, in readObject, if obj is null, the code should throw an IOException
because
it's not possible to create a SerialJavaObject with null has parameter
(because obj.getClass()
that implictly checks null in the constructor).
I made the change to readObject. I did not put an explicit check in the
constructor but will do that under a separate bug
I also added the comment to SerialDataLink and removed the read/writeObject
http://cr.openjdk.java.net/~lancea/8002212/webrev.02
Best
Lance
All other classes are Ok for me.
I also added SerialStruct to the webrev.
SerialStruct is Ok for me.
Have a great weekend.
Have a nice weekend too.
Best
Lance
cheers,
Rémi
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 8002212 - adding read/writeObject to additional SerialXXX classes -- Updated
On 11/03/2012 05:23 PM, Lance Andersen - Oracle wrote:
On Nov 3, 2012, at 11:34 AM, Remi Forax wrote:
On 11/03/2012 03:11 PM, Lance Andersen - Oracle wrote:
I revised the webrev, http://cr.openjdk.java.net/~lancea/8002212/webrev.01,
taking into account the vast majority of Remi's suggestions.
in SerialJavaObject, hasStaticFields doesn't work, the original code doesn't
work because
it only check for fields that are declared static but not for fields that are
by example public static.
private static boolean hasStaticFields(Field[] fields) {
for (Field field : fields) {
if ( Modifier.isStatic(field.getModifiers())) {
return true;
}
}
return false;
}
This may cause compatibility issue because despite the specification, the
original code
will let objects that have a static field to be serialized.
I cannot make the change above as it breaks too many tests and I would prefer
to go with the less is more scenario. As I think I mentioned before, I do not
think the original authors really thought through these classes and thankfully
they are not used much, if at all.
Also, in readObject, if obj is null, the code should throw an IOException
because
it's not possible to create a SerialJavaObject with null has parameter (because
obj.getClass()
that implictly checks null in the constructor).
I made the change to readObject. I did not put an explicit check in the
constructor but will do that under a separate bug
I also added the comment to SerialDataLink and removed the read/writeObject
http://cr.openjdk.java.net/~lancea/8002212/webrev.02
Ok, thumb up.
Best
Lance
cheers,
Rémi
All other classes are Ok for me.
I also added SerialStruct to the webrev.
SerialStruct is Ok for me.
Have a great weekend.
Have a nice weekend too.
Best
Lance
cheers,
Rémi
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
Am 30.10.2012 23:40, schrieb Xueming Shen:
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:-)
Yes, it doesn't matter much, but would be a nice conform style, so for me personally I would like
the copy/paste:-)
- What (should) happen(s), if lineSeparator.length == 0 ?
do nothing. expected?
I would say, empty line separator should not be allowed, so you might check:
Objects.requireNonEmpty(lineSeparator);
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.
It seems you have mixed 2 tweaks to one. ;-) See additional paragraph at the
end ...
but it appears the hotspot likes it the constant/fixed length lookup
table is inside encoder.
Yes, but see my suggestion 12 lines below.
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.
Are you really sure? As it only runs once at class init, JIT should not compile
this code.
Executing the bytecode to init the final int[], value for value, by interpreter should not be faster
as expanding a String source in a loop.
Those repeating lines of isURL? is also supposed to be a
performance tweak to eliminate the array boundary check in loop.
Yep, so I was thinking about:
653 private final int[] base64;
654 private final boolean isMIME;
655
656 private Decoder(boolean isURL, boolean isMIME) {
657 base64 = isURL ? fromBase64URL : fromBase64;
658 this.isMIME = isMIME;
659 }
.
911 private int decodeBuffer(ByteBuffer src, ByteBuffer dst) {
912 int[] base64 = this.base64; // local copy for performance reasons (but maybe
superfluous if HotSpot is intelligent enough)
or:
911 private int decodeBuffer(ByteBuffer src, ByteBuffer dst, int[]
base64) {
.
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.
925 if (b == '=') {
-- causes one additional if in the _main_ path of the loop, so should be
slower for regular input
859 if (b == -2) { // padding byte
-- causes one additional if in the _side_ path of the loop, so should only
affect irregular input
Maybe the following code is little faster as no loading of the constant '-2' is
required:
858 if ((b = base64[b]) 0) {
859 if (++b 0) { // padding byte '='
Once again (the following was meant for the decode algorithm, not
initialisation):
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. In an int[] table, the index offset must be shifted by 2
before. Maybe this doesn't directly affect the performance on x86/IA64 CPU, but wastes space in CPU
cache for other tasks as a side effect. On ARM architectures I imagine, directly addressing a
byte-stepped table could be faster than addressing a 4-byte-stepped table. At least the footprint of
the table would be smaller.
Little nit:
You could delete line 641 for conformity with 629.
-Ulf
hg: jdk8/tl/langtools: 8002146: javadoc doesn't release resources in a timely manner
Changeset: ef3ad754f5c7 Author:jjg Date: 2012-11-03 21:07 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/ef3ad754f5c7 8002146: javadoc doesn't release resources in a timely manner Reviewed-by: darcy ! src/share/classes/com/sun/tools/javadoc/JavadocMemberEnter.java ! src/share/classes/com/sun/tools/javadoc/Start.java
hg: jdk8/tl/langtools: 8002168: Cleanup initialization of javadoc Messager
Changeset: 352d130c47c5 Author:jjg Date: 2012-11-03 21:09 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/352d130c47c5 8002168: Cleanup initialization of javadoc Messager Reviewed-by: darcy ! src/share/classes/com/sun/tools/javadoc/Start.java ! test/tools/javadoc/6958836/Test.java
