[
https://issues.apache.org/jira/browse/THRIFT-248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12668689#action_12668689
]
Kevin Clark commented on THRIFT-248:
------------------------------------
I'm happy with the approach and the code looks good. Just a couple things:
I think you might have a bug in memory_buffer.c:51. You're not coercing the
value of GARBAGE_BUFFER_SIZE to an int from a ruby object, so I think you're
just getting the object id, which doesn't look like it's what you want.
It doesn't bother me if you want to push through RopeTransport as well, but
there's some commented out lines in reference to it in Serializer/Deserializer
we should clean up. Same in struct.rb#write
I don't think the reduction in speed is a big deal. It's not a large slowdown,
and improves things for other protocols. I think it makes writing the protocols
in C easier as well. What does this do with regards to people who haven't
compiled the binary? It looked like it was being automatically required
somewhere, though I might have imagined that. At this point I'm not sure if it
matters if we require the extension to be built or not. The build system tries
to do it as a default.
> Factor BinaryProtocolAccelerated into separate protocol and struct components
> -----------------------------------------------------------------------------
>
> Key: THRIFT-248
> URL: https://issues.apache.org/jira/browse/THRIFT-248
> Project: Thrift
> Issue Type: Improvement
> Components: Library (Ruby)
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Priority: Minor
> Attachments: thrift-248-v2.patch, thrift-248-v3.patch,
> thrift-248-v4.patch, thrift-248-v5.patch, thrift-248-v6.patch,
> thrift-248.patch
>
>
> Kevin Clark's excelled BinaryProtocolAccelerated implementation in the Ruby
> library is very fast, in large part due to the fact that it implements not
> just the protocol but also the struct components of serialization directly as
> a C extension. The problem with this arrangement is that other protocols that
> would benefit from accelerated struct code don't get the benefit. In
> particular, I'd like to make my implementation of the Compact Protocol fast
> in Ruby, and the key appears to be the struct serialization code.
> I think that we should make an effort to divorce the struct stuff from the
> protocol stuff in BinaryProtocolAccelerated, so that all protocols can
> benefit. Some quick benchmarking seems to indicate that there is going to be
> some additional method call overhead in this situation, but it's not really
> that substantial.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
