Add an externalizable serializer that uses ObjectInput and ObjectOutput ... #167
Conversation
…ut adapters but has the ability to switch when fancy serialization stuff is tried.
| int total = 0; | ||
| int cur = 0; | ||
|
|
||
| while ((total<n) && ((cur = (int) input.skip( (long)n-total) ) > 0)) { |
There was a problem hiding this comment.
I think Input#skip should be sufficient without the while loop.
This was a mistake. Something in progress.
|
Alright, I think I fielded all the comments. Let me know if I've missed anything. |
| /** | ||
| * Copyright (c) 2012,2013 | ||
| */ | ||
| public class ExternalizableMaySerializeSerializer extends ExternalizableSerializer { |
There was a problem hiding this comment.
This class name is not great. Why not have ExternalizableSerializer be able to fall back to JavaSerializer?
There was a problem hiding this comment.
The main reason for having two versions was to prevent from having to keep
a bunch of classes in memory and mapped to JavaSerializers as well as the
suppress "replace method" check option. Two classes seemed to be much more
memory conscious and performant. But yeah, I hear you on the name.
On Fri, Jan 10, 2014 at 10:45 AM, Nathan Sweet [email protected]:
In
src/com/esotericsoftware/kryo/serializers/ExternalizableMaySerializeSerializer.java:@@ -0,0 +1,67 @@
+package com.esotericsoftware.kryo.serializers;
+
+import com.esotericsoftware.kryo.Kryo;
+import com.esotericsoftware.kryo.io.Input;
+import com.esotericsoftware.kryo.io.Output;
+
+import java.lang.reflect.Method;
+
+/**
- * Copyright (c) 2012,2013
- */
+public class ExternalizableMaySerializeSerializer extends ExternalizableSerializer {This class name is not great. Why not have ExternalizableSerializer be
able to fall back to JavaSerializer?—
Reply to this email directly or view it on GitHubhttps://pull/167/files#r8797792
.
There was a problem hiding this comment.
The name is a no-go. There doesn't seem to be a map anymore, can the classes be combined?
There was a problem hiding this comment.
Okay, so what I'm thinking here is that the option to NOT fall-back on JavaSerialization is an unnecessary optimization. The simplest thing that could work here is to just make the fall back always checked, push this implementation into ExternalizableSerializer and just get rid of this class. Work?
There was a problem hiding this comment.
Also, just so I'm crystal clear.
- A single serializer does not need to be thread safe.
- A single serializer CAN be used on different classes. (Hence the need for a map)
Is that correct?
This reverts commit 31d15f8.
|
I too needed Kryo to implement ObjectInput / ObjectOutput, so I welcome this PR. |
| readFully( b, 0, b.length ); | ||
| } | ||
|
|
||
| public void readFully( byte[] b, int off, int len ) throws IOException { |
There was a problem hiding this comment.
why not use Input.readBytes(byte[], int, int) here? catch KryoException and convert to appropriate IOException
There was a problem hiding this comment.
It seemed like this would be both optimal and more proper if someone ever decided to implement an Input that used a socket or some such. I can change it to just call readBytes if you think it's an important improvement. I wasn't really sure what the difference is so I went for a stock readFully implementation.
There was a problem hiding this comment.
I believe readBytes should do the same as readyFully.
|
If we can address the comments above and run the source formatter, then we can merge. |
|
Can you point me to the source code formatter? On Fri, Mar 21, 2014 at 8:34 AM, Nathan Sweet [email protected]:
|
|
nathansweet is mentioned by name in libgdx's wiki section on formatting. I think thats the one |
|
Good enough. Using the code format settings from that project. On Fri, Mar 21, 2014 at 9:56 AM, mucaho [email protected] wrote:
|
|
The formatter settings are committed in the .settings Eclipse folder in the Kryo project. Source -> Format should run it using project specific settings. It looks slightly out of date since the comments weren't formatted quite right, but close enough. |
Add an externalizable serializer that uses ObjectInput and ObjectOutput ...
...adapters but has the ability to switch when fancy serialization stuff is tried.
I'm not super familiar with this library so I hope I kept the right code philosophy. The ability to branch to the JavaSerializer is disabled by default. But I added test cases to ensure it worked and properly did a readResolve.