Specialize the TBinaryProtocol read path up to ~2x speedups #221
Conversation
| } | ||
|
|
||
| override def apply(item: T) = thriftStructSerializer.toBytes(item) | ||
| override def invert(bytes: Array[Byte]) = attempt(bytes){ bytes => |
There was a problem hiding this comment.
what about Macros.fastAttempt here to avoid the closure. :) Maybe no win, but it can't hurt.
There was a problem hiding this comment.
Perf sensitive code, might as well take every little bit.
|
This isn't feasible for dependency reasons, but it would be nice if we didn't duplicate work. We have an implementation of TBinaryProtocol in finagle to minimize allocations: It would be neat to be able to use your optimizations when possible too. |
|
Yeah alas not obvious for that, a scrooge-thrift type package with slim deps might be a good middle ground to use for everything |
|
looks good. Nice work. |
Specialize the TBinaryProtocol read path up to ~2x speedups
| def readString: String = | ||
| try { | ||
| val size = readI32 | ||
| val s = new String(transport.buf, transport.bufferPos, size, "UTF-8") |
There was a problem hiding this comment.
you could try the trick used here:
https://github.com/apache/parquet-mr/blob/d6f082b9be5d507ff60c6bc83a179cc44015ab97/parquet-column/src/main/java/org/apache/parquet/io/api/Binary.java#L90
Charset.decode instead of new String
There was a problem hiding this comment.
It doesn't mention any relative performance numbers, much faster?
There was a problem hiding this comment.
If this is the one that I'm thinking of, up to 40x faster, but it only applies to java6 and was fixed in java7. But I could be wrong.
There was a problem hiding this comment.
Well we are moving to java8, so not sure it matters much if fixed in java7
On Thu, Jun 18, 2015 at 11:32 AM, Alex Levenson notifications@github.com
wrote:
In
bijection-thrift/src/main/scala/com/twitter/bijection/thrift/TArrayBinaryProtocol.scala
#221 (comment):
((transport.buf(off + 1) & 0xffL) << 48) |((transport.buf(off + 2) & 0xffL) << 40) |((transport.buf(off + 3) & 0xffL) << 32) |((transport.buf(off + 4) & 0xffL) << 24) |((transport.buf(off + 5) & 0xffL) << 16) |((transport.buf(off + 6) & 0xffL) << 8) |((transport.buf(off + 7) & 0xffL))- }
- def readDouble: Double =
- java.lang.Double.longBitsToDouble(readI64)
- def readString: String =
- try {
val size = readI32val s = new String(transport.buf, transport.bufferPos, size, "UTF-8")If this is the one that I'm thinking of, up to 40x faster, but it only
applies to java6 and was fixed in java7. But I could be wrong.—
Reply to this email directly or view it on GitHub
https://github.com/twitter/bijection/pull/221/files#r32762644.
There was a problem hiding this comment.
I can't really tell, a read a blog post just now that said they fixed it for single-byte charsets, but not yet for multi-byte charsets like utf8.
Can see the prior delta in the posted image. The macro stays below but the others that do full deserialization converge with this change.