close
Skip to content

support mutable BitSet#185

Merged
johnynek merged 1 commit into
twitter:developfrom
nevillelyh:develop
Jun 2, 2014
Merged

support mutable BitSet#185
johnynek merged 1 commit into
twitter:developfrom
nevillelyh:develop

Conversation

@nevillelyh
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@nevillelyh
Copy link
Copy Markdown
Contributor Author

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

@rxin
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@rxin
Copy link
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

rxin commented Jun 1, 2014

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

@nevillelyh
Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor

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