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

support mutable BitSet #185

Merged
merged 1 commit into from
Jun 2, 2014
Merged

support mutable BitSet #185

merged 1 commit into from
Jun 2, 2014

Conversation

nevillelyh
Copy link
Collaborator

No description provided.

@@ -150,6 +151,7 @@ class ScalaCollectionsRegistrar extends IKryoRegistrar {
// The above ListMap/HashMap must appear before this:
.forTraversableSubclass(Map.empty[Any,Any])
// here are the mutable ones:
.forSubclass[MBitSet](new MutableBitSetSerializer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we do .forClass here? subclass is pretty dangerous because the type information of which subclass you have is lost. If you want to support subclasses, we need to write the class name, instantiate that, and fill it back up.

@johnynek
Copy link
Collaborator

Thanks for doing this. Will merge after you change to avoid subclasses.

@nevillelyh
Copy link
Collaborator Author

Thanks. Have very little experience with Kryo before, ran across this with Spark.
Fixed forSubclass => forClass.

@rxin
Copy link
Contributor

rxin commented May 31, 2014

Thanks for submitting this. Can you look into the implementation of toImmutable to see how expensive it is? If it is expensive, I'm not sure if it is a good idea to do this transformation. If it is very cheap, maybe it is ok.

@nevillelyh
Copy link
Collaborator Author

Here's the impl for toImmutable, seems fairly cheap.
def toImmutable = immutable.BitSet.fromArray(elems)

@rxin
Copy link
Contributor

rxin commented May 31, 2014

What if we change the signature of BitSetSerializer to take scala.collection.BitSet, so it can be used to serialize both immutable and mutable bitsets and then you get around this new object allocation?

While you are at it, I'm wondering if you can replace the foreach loop in the serializer with a while loop - it will get higher performance ...

@nevillelyh
Copy link
Collaborator Author

Not sure how that works since s.c.BitSet is just an trait while we still need to deserialize to original version of (im)mutable impl.

I can do the for -> while change if you guys think it's worth it :)

@rxin
Copy link
Contributor

rxin commented Jun 1, 2014

I think the write side is entirely reusable, whereas on the read side you'd need need to pick the right mutable vs immutable BitSet to initialize. Basically it is best to avoid creating a whole immutable bitset, and then creating a mutable bitset from that with foreach.

@johnynek
Copy link
Collaborator

johnynek commented Jun 1, 2014

I actually think you just want this:
https://github.com/twitter/chill/blob/develop/chill-scala/src/main/scala/com/twitter/chill/Traversable.scala#L21

.forTraversableClass[Any, MBitSet[Any]](MBitSet.empty[Any], false)

I don't think you need to write your own serializer here, just register one for mutable sets.

@rxin
Copy link
Contributor

rxin commented Jun 1, 2014

Cool if there is already a built-in serializer for serializing general traversable collections.

@nevillelyh
Copy link
Collaborator Author

Revised. Thanks for pointing this out!

johnynek added a commit that referenced this pull request Jun 2, 2014
@johnynek johnynek merged commit 203eea4 into twitter:develop Jun 2, 2014
@johnynek
Copy link
Collaborator

johnynek commented Jun 2, 2014

Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants