Provide an opt-out for disabling the optimization of generics serialization #433

romix · 2016-06-11T21:51:12Z

The existing optimization of generics serialization aims at reducing the size of the serialized representation. This optimization is enabled by default. But sometimes it may have a negative impact on performance. And sometimes it cannot handle complex cases of generics usage.

This patch changes the default behavior and disables this this optimization. To enable it, simply invoke FieldSerializerConfig.setOptimizedGenerics(true).

magro · 2016-06-12T22:03:46Z

AFAICS the following issues might benefit from this PR and should be checked with the changes of this PR: #377, #384 (#385), #411

Updates:

the test posted with Kryo does not correctly support parameterized type hierarchy #377 works with this PR and optimized generics disabled
the test added with [BugFix] Reset the serializer generics when returning a previously read object #385 works with optimized generics disabled, so ClassCastException When Writing a Map After Previously Reading a Map with Different Generic Type #384 seems to be fixed with this PR.

magro · 2016-06-12T22:08:56Z

src/com/esotericsoftware/kryo/serializers/FieldSerializer.java

 		CachedField cachedField;

-		if (fieldGenericType == fieldClass[0]) {
+		if (!config.isOptimizedGenerics() || fieldGenericType == fieldClass[0]) {
 			// This is a field without generic type parameters


This comment should probably be changed now.

romix · 2016-06-12T22:18:18Z

@magro I'm not sure about the interplay of the config field of the FieldSerializer and my patch. If someone would change the optimizedGenerics setting of the config from true to false after a FieldSerializer was build using this config, then it could lead to undefined behavior.

May be FieldSerializer should get a non-modifiable copy of the config?

magro · 2016-06-12T22:18:35Z

test/com/esotericsoftware/kryo/SerializationCompatTest.java

+        );
+        kryo.getFieldSerializerConfig().setOptimizedGenerics(false);
+        runTests(
+                "standard-nonopt-generics",


The new files (test/resources/*-standard-nonopt-generics.ser) created when running this SerializationCompatTest should be added to git (applies to other new nonopt files as well).

magro · 2016-06-12T22:39:44Z

@romix Thanks again for your work here! Can you elaborate a bit more on the consequences of the optimizedGenerics setting being true or false (e.g. with some examples and size/time measurements)? If you don't have the time for this I could also help with building a simple benchmark.
Update: we should also mention this setting in the README/FieldSerializer section.

As I understand it, with non-optimized generics we should have faster (de)serialization of generics but increased size, and generics serialization should be more robust. Assuming that generics serialization is in-fact more robust with this setting, I wonder if we should make this the default (if we'd release this then as 4.0 or 3.1 is a different question). WDYT?

/cc @NathanSweet

magro · 2016-06-12T23:18:07Z

@romix The FieldSerializer could also "freeze" this setting in the cloned
config by setting a special flag, so that the setter throws a exception if
called on the clone.

romix notifications@github.com schrieb am Mo., 13. Juni 2016, 00:18:

@magro https://github.com/magro I'm not sure about the interplay of the
config field of the FieldSerializer and my patch. If someone would change
the optimizedGenerics setting of the config from true to false after a
FieldSerializer was build using this config, then it could lead to
undefined behavior.

May be FieldSerializer should get a non-modifiable copy of the config?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#433 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAD7HbmVdZxuTv3e2gDBOLyPnTWmkoDfks5qLIWrgaJpZM4IzpmB
.

romix · 2016-06-13T14:52:58Z

@magro Regarding the consequences of this patch on size/performance: I haven't written or executed any benchmarks due to the lack of time. But your understanding is correct. I expect exactly the same as you do: with non-optimized generics we should have faster (de)serialization of generics but increased size, and generics serialization should be more robust. Basically, the size is likely the be roughly the same as it used to be before I introduced those generics optimizations a while ago.

It would be nice if you or someone else could help with the benchmarking of this non-optimized mode.

As for making it a default, I'm not sure. But it could be a good idea, indeed. Though, it would be a breaking change, because it changes the default binary serialization format in case of generics. People willing to proceed with the existing format will have to change their source code and enable the setting by hand.

magro · 2016-06-17T19:29:16Z

@romix Sorry for the late response, this week was very busy.

I'm going to submit a PR with my comments / proposed changes against your branch. If you're already working on it please ping me now so that we don't do double work.

Not sure if I'll have time for the benchmark as well, perhaps I'll submit a separate PR later against master.

Making the non-optimized mode the default: this is my preference, as it should improve stability and reduce submitted issues. I'll ask on the mailing list for opinions, let's see if there's feedback.

magro · 2016-06-17T22:21:10Z

@romix I just submitted my feedback as PR. If you think all this is fine you could merge + rebase/squash commits to a single one - and IMO we could merge it then. The most critical thing is that change for the optimizedGenerics default value :-)

romix · 2016-06-18T16:02:42Z

@magro Thanks a lot for your PR!

romix · 2016-06-22T20:23:27Z

@magro I force-pushed the new version of the patch. It includes all of your changes and my changes to avoid any hacks related to freezing of the FieldSerializerConfig.setOptimizedGenerics.

Please check if everything is OK and we could merge this.

magro · 2016-06-25T00:08:53Z

@romix Sorry for the late response... Because optimization is off by default the commit message is wrong in this regard. Can you update it? The readme probably should also mention this setting, but I could write that later as well.

… for enabling/disabling it. The existing optimization of generics serialization aims at reducing the size of the serialized representation. This optimization is enabled by default. But sometimes it may have a negative impact on performance. And sometimes it cannot handle complex cases of generics usage. This patch changes the default behavior and disables this this optimization. To enable it, simply invoke `FieldSerializerConfig.setOptimizedGenerics(true)`.

romix · 2016-06-25T03:58:22Z

@magro Good catch! I updated the commit message. The readme could be updated later, when we roll out a new release.

magro · 2016-06-25T06:09:39Z

So it's done now, great work!!

romix · 2016-07-01T20:51:40Z

@magro Martin, I'm not sure that closing all those related issues is the right thing to do. I agree that with #433 they are not present in the non-generic-optimized mode. But they are still present when generic optimizations are enabled. Therefore, I'd suggest tagging them with "optimize-generics" or something like this, instead of closing. WDYT?

magro · 2016-07-02T06:35:42Z

@romix Sounds good!

romix changed the title ~~Provide an opt-out for disabling the optimization of generics seriali…~~ Provide an opt-out for disabling the optimization of generics serialization Jun 11, 2016

magro reviewed Jun 12, 2016
View reviewed changes

magro mentioned this pull request Jun 17, 2016

Timeline for kryo point release #431

Closed

romix force-pushed the wip-opt-out-generics branch from c83161a to 0f05362 Compare June 22, 2016 20:20

romix force-pushed the wip-opt-out-generics branch from 0f05362 to 9923d05 Compare June 25, 2016 03:56

magro merged commit 5793300 into EsotericSoftware:master Jun 25, 2016

nkconnor mentioned this pull request Jul 1, 2016

Requires patch for generics serialization altoo-ag/akka-kryo-serialization#92

Closed

MarcMil mentioned this pull request Jul 30, 2019

Attempt to work around problems with generics #683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide an opt-out for disabling the optimization of generics serialization #433

Provide an opt-out for disabling the optimization of generics serialization #433

romix commented Jun 11, 2016 •

edited

magro commented Jun 12, 2016 •

edited

magro Jun 12, 2016

romix commented Jun 12, 2016

magro Jun 12, 2016 •

edited

magro commented Jun 12, 2016 •

edited

magro commented Jun 12, 2016

romix commented Jun 13, 2016

magro commented Jun 17, 2016

magro commented Jun 17, 2016

romix commented Jun 18, 2016

romix commented Jun 22, 2016

magro commented Jun 25, 2016

romix commented Jun 25, 2016

magro commented Jun 25, 2016

romix commented Jul 1, 2016

magro commented Jul 2, 2016

Provide an opt-out for disabling the optimization of generics serialization #433

Provide an opt-out for disabling the optimization of generics serialization #433

Conversation

romix commented Jun 11, 2016 • edited

magro commented Jun 12, 2016 • edited

magro Jun 12, 2016

Choose a reason for hiding this comment

romix commented Jun 12, 2016

magro Jun 12, 2016 • edited

Choose a reason for hiding this comment

magro commented Jun 12, 2016 • edited

magro commented Jun 12, 2016

romix commented Jun 13, 2016

magro commented Jun 17, 2016

magro commented Jun 17, 2016

romix commented Jun 18, 2016

romix commented Jun 22, 2016

magro commented Jun 25, 2016

romix commented Jun 25, 2016

magro commented Jun 25, 2016

romix commented Jul 1, 2016

magro commented Jul 2, 2016

romix commented Jun 11, 2016 •

edited

magro commented Jun 12, 2016 •

edited

magro Jun 12, 2016 •

edited

magro commented Jun 12, 2016 •

edited