Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kryo is reset twice when used in a pool #754

Closed
theigl opened this issue Aug 6, 2020 · 6 comments · Fixed by #758
Closed

Kryo is reset twice when used in a pool #754

theigl opened this issue Aug 6, 2020 · 6 comments · Fixed by #758

Comments

@theigl
Copy link
Collaborator

theigl commented Aug 6, 2020

Kryo 5 has a Poolable interface for objects that can be used with Pool.

When such an object is returned to the pool, Poolable.reset() is called:

/** Called when an object is freed to clear the state of the object for possible later reuse. The default implementation calls
 * {@link Poolable#reset()} if the object is {@link Poolable}. */
protected void reset (T object) {
	if (object instanceof Poolable) ((Poolable)object).reset();
}

This makes using Input und Output much easier in combination with pools.

The problem is that the Kryo class implements Poolable as well. So if autoReset is enabled (the default), Kryo.reset() will be called twice. First by auto-reset at the end of serialization, then when the object is returned to the pool.

This operation is potentially expensive, especially when references are enabled and class registration is not required.

We have two options to fix this:

  1. Do not let Kryo implement Poolable
  2. Document that autoReset should be disabled when Kryo is used in a pool

@magro: What do you think? We should address this before releasing 5.0 Final.

@magro
Copy link
Collaborator

magro commented Aug 11, 2020

For option 1 (Do not let Kryo implement Poolable) I'd say things would work as expected.
Probably I'm missing something - are there any drawbacks?

@theigl
Copy link
Collaborator Author

theigl commented Aug 11, 2020

Probably I'm missing something - are there any drawbacks?

There is only one potential issue: If autoReset is disabled and Kryo is not reset manually before it is returned to the pool. Currently, the pool would ensure that the Kryo instance is always reset.

I don't have a strong opinion on this. I'm OK with either option.

@magro
Copy link
Collaborator

magro commented Aug 11, 2020

Erm, right 😉

I'd think that users turning off autoReset do this for a good reason, for them it should be ok to manually reset the Kryo instance before returning it to the pool. We should only document this so that this is clear.

So I'd prefer option 1.

@theigl
Copy link
Collaborator Author

theigl commented Aug 11, 2020

Agreed 😉

I'll create a PR tomorrow morning.

@magro
Copy link
Collaborator

magro commented Aug 11, 2020

👍 Awesome!

theigl added a commit to theigl/kryo that referenced this issue Aug 12, 2020
…is not called twice after each (de)serialization
@theigl
Copy link
Collaborator Author

theigl commented Aug 12, 2020

@magro: I just pushed #758.

theigl added a commit to theigl/kryo that referenced this issue Aug 13, 2020
…is not called twice after each (de)serialization
magro added a commit that referenced this issue Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants