ReflectASM doesn't use primitive serializers

[NathanSweet]

From here:
https://groups.google.com/forum/#!topic/kryonet-users/AspUPsZcNu0

It seems ObjectField$ObjectIntField uses ObjectField#write/read which uses the registered serializer for primitive int. However AsmCachedField$AsmIntField does not use a serializer. This means something written on the desktop via ASM cannot be read on Android without ASM.

Here is an example:
https:/EsotericSoftware/kryonet/blob/master/src/com/esotericsoftware/kryonet/rmi/ObjectSpace.java#L594
ObjectField#write will use this serializer, AsmIntField#read won't.

[romix]

Oops. Very good catch! :-) It looks like I forgot during my re-factorings to overload the "write", "read" and "copy" methods for ObjectField when I was defining ObjectField$ObjectIntField and the like.

But which behavior do we prefer? The one from AsmCachedField-derived classes or the one from ObjectField-derived classes? I.e. do we want to use a registered serializer for primitive types or directly shortcut it and use output methods for primitive types? I'm in favor of the later more optimized approach.

[NathanSweet]

The optimized approach is fine until you want to customize the serialization of a primitive type. Maybe you know an int can't be negative or a string (not primitive but handled the same way) is always ASCII.

The optimization could be configurable, maybe by using a separate CachedFieldFactory. Actually I see the factory is internal and not configurable. Maybe it should be? It may simplify FieldSerializer if it just has a factory it gets fields from. FieldSerializer wouldn't need to know about ASM/unsafe/etc at all. Also the code for ASM/unsafe/etc would be separated into different factories, which could be nice and tidy. Maybe we should also remove asmEnabled from Kryo? It seems FieldSerializer specific, or, if CachedFieldFactory is made public then it is specific to the CachedFieldFactory implementation.

I see CachedFieldFactory is public but should be package private, unless we do the refactoring mentioned.

Anyway, maybe we should default to using the serializers, as that seems like the expected behavior, and have the optimization be configurable somehow? What do you think?

[romix]

Hmm. You touch on many interesting points.

1. Regarding removing asmEnabled from Kryo: On the one hand side, I agree with you. On the other hand side, usually this needs to be set globally for all FieldSerialzer instances. Doing it on Kryo instance makes it easy to automatically pass this flag to all FieldSerializers which will be created during registration. If we move it into FieldSerializer, we need to do it on a per FieldSerializer instance basis, which is annoying. If we move it into a Factory class, we probably need to be able to get hold of this factory easily, so that we can configure it. But how do we get access to this factory? Do we expose some sort of getFactory APIs? Where do we expose it, in Kryo?

2. The code for ASM/Unsafe/etc is separated into different factories already, or?

3. But your idea with FieldSerializer just having a configurable factory is nice. I'll look if it is possible without too much of refactoring. But it smells like it would need a not so trivial refactoring.

4. The most important thing: I'm not sure that using serializers for primitives is a very good idea.
   For one, we do not follow the idea of using serializers for primitive types in many places: DefaultArraySerializers, FieldSerializer and probably many more. Usually, these serializers are used for java.lang.Integer/etc classes.
   And I'm pretty sure that if we switch to using these serializers for primitive types, we would really hurt the performance. E.g. serialization of arrays would become slower, serialization of classes with fields of primitive types would become slower, etc. And IMHO this is a very high price to pay.

[NathanSweet]

1. Kryo has a SerializerFactory which can be used to create and configure the default serializers. This is where a user would set a FieldFactory on the FieldSerializer to choose if they wanted to use ASM, unsafe, etc. This way Kryo doesn't need knowledge (the asmEnabeld field) about something that is really specific to a particular serializer (which may not even be used).

2. Mostly, but FieldSerializer makes decisions about using ASM, unsafe, etc. This would move to the FieldFactory and simplify FieldSerializer.

3. Aye, the refactoring is probably not trivial.

4. The biggest value for customizing primitives is when you know an int can't be negative or when you know a String is ASCII. I guess I would be fine with disallowing this customization considering its cost, but currently the API makes it look like serialization for any type is allowed. Having primitive (and primitive wrapper) serializers that are only used sometimes is unintuitive. Having serializers only used for certain modes (eg the issue above, ASM vs non-ASM) is broken.

[romix]

1. Thanks for clarifications about factories. I never used them so far, but now I understand how it is supposed to work. I agree that it makes Kryo internals cleaner. But I'm not quite convinced that an average user who just wants to use e.g. Unsafe-based FieldSerializer instead of ASM-based ones will be able to easily write a code that defines a custom SerializerFactory and set it as a default factory for a Kryo instance. Basically, I have the impression it should be API-wise more user-friendly. Or may be we should provide examples/snippets in the docs. Right now we don't even mention SerializerFactory in our docs, IIRC.

2. Having serializers only used for certain modes (eg the issue above, ASM vs non-ASM) is broken.
   While I understand your point, I think it is also mostly OK to expect that users configure Kryo exactly in the same way on the serializing and deserializing side. Most other frameworks do it as well, I think. And if configured in the same way (e.g. ASM on both sides or the same kind of non-ASM on both sides), there should be no troubles. So far, we almost never had bug reports about incompatibilities of this kind, or?

But coming back to the original issue:

• The idea of using factories is good, but will take some time before it is implemented.
• Should I try as a short-term solution to align the ASM and Object-based FieldSerialier.XXXField classes for primitive types in the mean-time, so that they behave in the same way? More specifically, I'd suggest that both do not use registered serializers for primitive fields and instead use the corresponding output methods directly

[romix]

Ping?

[NathanSweet]

Sorry, missed this one.

1. More abstraction layers always means a nastier API. Possibly we could provide a few factories out of the box.

2. True, though in this case the use of ASM is automatically done internally, so from the users perspective they have configured it the same. Maybe we need less magic in the background, we could have ASM enabled by default and we don't automatically disable it? ASM usage where it isn't supported will throw an exception, making it clear that some configuration is needed.

3. Sure, a stop gap solution is better than nothing.

[romix]

Nate, I committed changes that make all FieldSerializer backends behave in the same way, when it comes to serialization of primitive types. So, at least it should be possible now to serialize on desktop and deserialize on Android and vice versa, I hope. Would be nice if you could check it.

BTW, can we somehow disable usage of ASM even if it is available and force usage of a reflection-based backend? This would allow us to write unit tests which check ASM, Unsafe and Reflection-based backends.

[NathanSweet]

Cool, thanks! I don't have a test setup unfortunately, the issue came from a KryoNet user.

Disabling ASM could be done by the FieldFactory stuff discussed above. The default factory could choose the actual factory based on what is available, otherwise a user could configure a FieldSerializer to use a ReflectionFieldFactory, for example.

[magro]

Closing, reopen if still relevant.