Fix for #180 #184
Fix for #180 #184
Conversation
Rebase on current Kryo trunk
|
Cool, thanks a ton! Since we have to annotate every domain object in our app will this potentially get expensive in creating serializer objects? Does the following make sense to add for potential performance improvement: // here we capture a specific serializer for a particular field
if (field.isAnnotationPresent(BindKryoSerializer.class)) {
Class<? extends Serializer> serializerClass = field.getAnnotation(BindKryoSerializer.class).value();
Serializer setSerializer = super.getKryo().getSerializer(field.getClass());
if (serializerClass.isInstance(setSerializer)) {
fields[i].setSerializer(setSerializer);
} else {
Serializer s = ReflectionSerializerFactory.makeSerializer(super.getKryo(), serializerClass,
field.getClass());
fields[i].setSerializer(s);
}
}Basically it doesn't use the reflection factory to create the serializer if it is already that serializer. Not sure if this makes sense in the grand scheme of things. Thanks again! |
|
@pheer Thanks for your comment. Your idea of checking for known serializers is good. But I'm not sure that the way how you perform this check and reuse it is correct. Basically, it reuses the same instance of a serializer. And this is not allowed in most cases. Instead, one should probably ask Kryo to create a new instance of a serializer for a registered class. It can still be cheaper or at most as expensive as using a reflection factory. Another possible optimization: We can cache serializer classes seen so far. |
|
Okay, that makes sense. Thanks! |
| static public class MultipleTimesAnnotatedCollectionFields { | ||
| // This annotation should result in an exception, because | ||
| // it is applied to a non-collection field | ||
| @CollectionSerializer.BindKryoSerializer( |
There was a problem hiding this comment.
Seems a bit verbose. Maybe just "CollectionSerializer.Bind("?
There was a problem hiding this comment.
I agree that is a bit verbose. I renamed it into "Bind" now. But I see one small issue: If someone uses CollectionSerializer.Bind, MapSerializer.Bind and FieldSerializer.Bind in the same file then only one of them can be used without a class name prefix (i.e. without XXXSerializer) and all others have to use it, because the name of the annotation is the same. Is it bad? May be we should have BindCollection, BindMap and Bind as names instead? Then users may use these shortcuts without class names in front of them.
|
Seems ok to me. We seem to loop through all the fields in FieldSerializer multiple times. FieldSerializer in general has become a scary monster for me, I hardly recognize it. I wonder if it can be refactored to separate logic into individual components. |
|
@pheer: Regarding the optimization of serializer instances creation: There are some possibilities, but they would make the code more complex. So I'm wondering if it is a premature optimization? After all, serializers are created only once, when a FieldSerializer is constructed. During serialization/deserialization no new serializer instances are created. Have you really detected that this code becomes a bottleneck in your application? |
|
@romix: no I just noticed it. I agree this may be a premature On Fri, Apr 4, 2014 at 8:29 AM, romix [email protected] wrote:
|
|
@pheer OK. Thanks for your reply. BTW, what do you think about the issue with naming of annotations I mentioned above? I mean this: #184 (comment) |
|
@NathanSweet @pheer Ping! If possible, I'd like to hear your opinion about annotation names before I provide a final version PR. |
|
Ping! |
|
Sorry I was out of network range for a while (needed to get away from On Thu, Apr 17, 2014 at 8:17 AM, romix [email protected] wrote:
|
Hi, please find for your review the initial support for field annotations (#180). It is fully functional and I provided unit tests for it as part of FieldSerializerTest.
Any comments are welcome!