[
https://issues.apache.org/jira/browse/THRIFT-1035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12980383#action_12980383
]
Bryan Duxbury commented on THRIFT-1035:
---------------------------------------
bq. ByteBuffer are not Thread Safe and thus would impose extraneous precautions
on their use.
Neither are byte[], in the sense that you can easily clobber the contents being
used by other threads if you're not being careful. Besides, in 99.9% of cases,
the ByteBuffer is going to be wrapped around a byte[], so you can use it just
as thread-safely.
bq. I agree that my fix introduces an inconsistency internally, but on the user
facing side we've worked after THRIFT-830 so binary fields could be
set/retrieved using byte[], right now collections of binary fields cannot use
byte[], so we suffer from a lack of external consistency which, in my point of
view, is more important to have than internal consistency.
I'm actually not talking about internal consistency. I'm talking about
consistently giving users a way to access the ByteBuffer version if that's what
they want. Overall, if we were creating Thrift Java from scratch today,
*everything* would be using ByteBuffer. We're only supporting the byte[] getter
for backwards compatibility. From our discussion, I think that the cost of
giving a backwards compatible interface to collections is very high, and that's
why I'm counseling that we don't do it.
bq. On the performance side, could you pinpoint cases where using byte[] in
collections would be less efficient that the ByteBuffers, since the retrieved
arrays will be those backing the ByteBuffers when deserializing and the
ByteBuffer passed to the serialization code are simply wrapping the byte[] in
the collections.
If you're putting a collection *into* a Thrift struct, then you're correct -
there is no positive performance impact of using ByteBuffers over byte[]
(unless you already have ByteBuffers from NIO operations...). However, on the
read side, having to read into a byte[] when you could just tag the underlying
buffer with a ByteBuffer is a performance loss. That's what I'm referring to.
If we make all collections use byte[] instead of ByteBuffer, you never get the
ByteBuffer read benefits.
> Container types containing binary data are parameterized with ByteBuffer in
> the generated Java code
> ---------------------------------------------------------------------------------------------------
>
> Key: THRIFT-1035
> URL: https://issues.apache.org/jira/browse/THRIFT-1035
> Project: Thrift
> Issue Type: Bug
> Components: Java - Compiler, Java - Library
> Affects Versions: 0.4, 0.5, 0.6, 0.7
> Environment: All
> Reporter: Mathias Herberts
> Attachments: THRIFT-1035-2.patch, THRIFT-1035.patch
>
>
> Since THRIFT-830, binary fields are internally handled using ByteBuffer.
> Release 0.4.0 was the first to expose the ByteBuffer to the outside world
> (replacing previous methods returning/accepting byte[]).
> THRIFT-882 lead to the methods accepting/returning byte[] being available
> again, as it was deemed more reasonable not to expose the ByteBuffer too much
> as their use could be cumbersome. This lead to 0.5.0 being backward
> compatible with 0.3.0 on the binary fields front.
> During that time, nobody noticed that container types that contained binary
> data had their generated Java code changed to collections parameterized with
> ByteBuffer instead of byte[].
> list<binary> -> List<ByteBuffer>
> set<binary> -> Set<ByteBuffer>
> map<binary,...> -> Map<ByteBuffer,...>
> map<...,binary> -> Map<...,ByteBuffer>
> This introduces confusion in the API and still exposes ByteBuffer when
> discussion on THRIFT-882 concluded this should be avoided.
> We need to provide a way to offer the original parameterization with byte[]
> as this will simplify working with that type of collection and thus will
> increase the odds of Thrift's adoption.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
