add Buf <-> Array[Byte] bijection #208
Conversation
|
|
||
| implicit def byteArrayBufBijection: Bijection[Array[Byte], Buf] = | ||
| new AbstractBijection[Array[Byte], Buf] { | ||
| override def apply(bytes: Array[Byte]) = Buf.ByteArray.Owned(bytes) |
There was a problem hiding this comment.
idk should this be owned or shared?
There was a problem hiding this comment.
The ChannelBuffer bijection wraps the byte array on application and copies on extraction so this jives with the wrapping half but not the extraction.
I can see a case for defining both versions and letting folks import the semantics they prefer? otoh, if users care about defensive copies, they can fairly trivially compose that onto any use of this version.
I do think making the shared impl the default is a mistake since this code tends to be used for serialization in batch jobs and as the hyper-doop folks showed is frequently a performance bottleneck.
There was a problem hiding this comment.
Hmm, maybe we should namespace them slightly differently and supply both? I feel like the point of Owned vs Shared was that we wanted folks to have to think about what it was OK to do with the data, so maybe we should do it here too (since we intend for this to be an extension of our API).
|
Looks fine to me, but best to resolve with Moses about Owned or Shared, I don't have enough context on Buf to know the best answer there. |
| @@ -122,6 +123,22 @@ trait UtilBijections { | |||
| override def apply(pool: FuturePool) = new TwitterExecutionContext(pool) | |||
| override def invert(context: ExecutionContext) = new ScalaFuturePool(context) | |||
| } | |||
|
|
|||
| object Owned { | |||
There was a problem hiding this comment.
can we get some comments here to explain the differences? I guess in one case (shared?) you are copying the underlying buffer, but in the other you are not, which only makes it safe if you "own" the Buf/Array[Byte] in question. Is that right?
There was a problem hiding this comment.
That's right, it describes ownership of the underlying byte array with implications of defensive copying. I'll add a note.
There was a problem hiding this comment.
we now have objects Owned and shared under twitter_util, the namespacing there doesn't make too much sense. A sub package or object to put them into a Buf namespace so it aligns more with util I think would be good here
There was a problem hiding this comment.
actually, could we add Shared / Owned type aliases, and then use different type classes for .as[Shared] vs .as[Owned] or is scala too smart for that?
There was a problem hiding this comment.
I don't believe that'll work anyway -- You could though add them in as AnyVal case classes here, and make the injections be to those. It would be a similar strategy as to how the Base64String stuff is done
There was a problem hiding this comment.
something like:
case class Owned(bytes: Array[Byte]) extends AnyVal
case class Shared(bytes: Array[Byte]) extends AnyValthen you can do buf.as[Shared].bytes if we want to go that way. I kind of like usage of types for this kind of decoration.
There was a problem hiding this comment.
Those wouldn't work for APIs that need to take Bufs though, right? You'd need to do bytes.as[Owned].unwrap, which is practically as bad as
val Buf.Owned(bytes) = buf
val buf = Buf.Owned(bytes)I think @ianoc's idea for a Buf.Owned namespace is reasonable.
add Buf <-> Array[Byte] bijection
|
Thanks @dschobel ! we're going to get this out in the next few days. |
This PR defines a bijection between Array[Byte] and com.twitter.io.Buf with a bonus version bump of the util-core lib.