Skip to content

Conversation

@bergander
Copy link
Contributor

This PR adds support for serializing sets created by ConcurrentHashMap.newKeySet().

@bergander bergander force-pushed the AddKeySetViewSerializer branch 6 times, most recently from 4d8c1ca to 0d8ff23 Compare May 5, 2025 12:04
@bergander bergander force-pushed the AddKeySetViewSerializer branch from 0d8ff23 to 1e6f4a3 Compare May 6, 2025 06:48
@bergander bergander force-pushed the AddKeySetViewSerializer branch from 1e6f4a3 to 5e8938f Compare May 6, 2025 06:51
/** Serializer for {@link ConcurrentHashMap.KeySetView}.
* @author Andreas Bergander */
public static class KeySetViewSerializer extends CollectionSerializer<ConcurrentHashMap.KeySetView> {
protected void writeHeader (Kryo kryo, Output output, ConcurrentHashMap.KeySetView set) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that this is not ideal. We are writing the keys twice. Once here as part of the map in the header and then again in the main write method of the CollectionSerializer. It would be enough to write the map because the keyset is only a view. Can you try to implement this without extending CollectionSerializer and implement Serializer directly? This should make the serialized payload significantly smaller.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writeHeader would simply become write and create would become read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing me in the right direction! PR is updated according to your comments.

@bergander bergander force-pushed the AddKeySetViewSerializer branch from c23345f to 899dd2d Compare May 6, 2025 09:11
@bergander bergander force-pushed the AddKeySetViewSerializer branch from 899dd2d to 96015bd Compare May 6, 2025 09:16
Copy link
Collaborator

@theigl theigl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks great now, thanks!

@theigl theigl merged commit 23fbc2d into EsotericSoftware:master May 6, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants