From: David Reiss Date: Sat, 2 Feb 2008 00:54:48 +0000 (+0000) Subject: Thrift: Update the interface for TTransport's "borrow" method. X-Git-Tag: 0.2.0~1018 X-Git-Url: https://source.supwisdom.com/gerrit/gitweb?a=commitdiff_plain;h=d46eb09befca3e43b01a00476611c7a0c0fc0254;p=common%2Fthrift.git Thrift: Update the interface for TTransport's "borrow" method. Summary: I don't know what I was thinking when I first wrote this. It makes sense that the transport might not want to allocate its own memory, so the protocol is expected to provide a buffer for the data. However, if the transport already has the data buffered, there is no need to memcpy it; it can just return a pointer into its buffer. The new interface still requires the protocol to provide a buffer, but allows the transport to return a pointer to an interal buffer. In addition, I made len a pass-by-pointer parameter so that the transport can return more than the requested data if it has it available in its buffers. Reviewed By: mcslee Test Plan: Ran the DenseProtocol test and the Zlib test. Revert Plan: ok Other Notes: Also got this reviewed by Chad Walters from Powerset. Ben Maurer suggested making len a reference parameter. git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@665454 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/lib/cpp/src/protocol/TDenseProtocol.cpp b/lib/cpp/src/protocol/TDenseProtocol.cpp index e79d4f1d..55f39028 100644 --- a/lib/cpp/src/protocol/TDenseProtocol.cpp +++ b/lib/cpp/src/protocol/TDenseProtocol.cpp @@ -180,12 +180,13 @@ inline uint32_t TDenseProtocol::vlqRead(uint64_t& vlq) { uint32_t used = 0; uint64_t val = 0; uint8_t buf[10]; // 64 bits / (7 bits/byte) = 10 bytes. - bool borrowed = trans_->borrow(buf, sizeof(buf)); + uint32_t buf_size = sizeof(buf); + const uint8_t* borrowed = trans_->borrow(buf, &buf_size); // Fast path. TODO(dreiss): Make it faster. - if (borrowed) { + if (borrowed != NULL) { while (true) { - uint8_t byte = buf[used]; + uint8_t byte = borrowed[used]; used++; val = (val << 7) | (byte & 0x7f); if (!(byte & 0x80)) { diff --git a/lib/cpp/src/transport/TTransport.h b/lib/cpp/src/transport/TTransport.h index b3d88206..4e3c218b 100644 --- a/lib/cpp/src/transport/TTransport.h +++ b/lib/cpp/src/transport/TTransport.h @@ -139,21 +139,31 @@ class TTransport { virtual void flush() {} /** - * Attempts to copy len bytes from the transport into buf. Does not consume - * the bytes read (i.e.: a later read will return the same data). This - * method is meant to support protocols that need to read variable-length - * fields. They can attempt to borrow the maximum amount of data that they - * will need, then consume (see next method) what they actually use. Some - * transports will not support this method and others will fail occasionally, - * so protocols must be prepared to use read if borrow fails. + * Attempts to return a pointer to \c len bytes, possibly copied into \c buf. + * Does not consume the bytes read (i.e.: a later read will return the same + * data). This method is meant to support protocols that need to read + * variable-length fields. They can attempt to borrow the maximum amount of + * data that they will need, then consume (see next method) what they + * actually use. Some transports will not support this method and others + * will fail occasionally, so protocols must be prepared to use read if + * borrow fails. * - * @oaram buf The buffer to store the data - * @param len How much data to borrow - * @return true if the requested data has been borrowed, false otherwise + * @oaram buf A buffer where the data can be stored if needed. + * If borrow doesn't return buf, then the contents of + * buf after the call are undefined. + * @param len *len should initially contain the number of bytes to borrow. + * If borrow succeeds, *len will contain the number of bytes + * available in the returned pointer. This will be at least + * what was requested, but may be more if borrow returns + * a pointer to an internal buffer, rather than buf. + * If borrow fails, the contents of *len are undefined. + * @return If the borrow succeeds, return a pointer to the borrowed data. + * This might be equal to \c buf, or it might be a pointer into + * the transport's internal buffers. * @throws TTransportException if an error occurs */ - virtual bool borrow(uint8_t* buf, uint32_t len) { - return false; + virtual const uint8_t* borrow(uint8_t* buf, uint32_t* len) { + return NULL; } /** diff --git a/lib/cpp/src/transport/TTransportUtils.cpp b/lib/cpp/src/transport/TTransportUtils.cpp index b10958cb..b5e7fa85 100644 --- a/lib/cpp/src/transport/TTransportUtils.cpp +++ b/lib/cpp/src/transport/TTransportUtils.cpp @@ -59,15 +59,15 @@ void TBufferedTransport::write(const uint8_t* buf, uint32_t len) { } } -bool TBufferedTransport::borrow(uint8_t* buf, uint32_t len) { +const uint8_t* TBufferedTransport::borrow(uint8_t* buf, uint32_t* len) { // Don't try to be clever with shifting buffers. - // If we have enough data, give it, otherwise - // let the protcol use its slow path. - if (rLen_-rPos_ >= len) { - memcpy(buf, rBuf_+rPos_, len); - return true; + // If we have enough data, give a pointer to it, + // otherwise let the protcol use its slow path. + if (rLen_-rPos_ >= *len) { + *len = rLen_-rPos_; + return rBuf_+rPos_; } - return false; + return NULL; } void TBufferedTransport::consume(uint32_t len) { @@ -203,15 +203,15 @@ void TFramedTransport::flush() { transport_->flush(); } -bool TFramedTransport::borrow(uint8_t* buf, uint32_t len) { +const uint8_t* TFramedTransport::borrow(uint8_t* buf, uint32_t* len) { // Don't try to be clever with shifting buffers. - // If we have enough data, give it, otherwise - // let the protcol use its slow path. - if (read_ && (rLen_-rPos_ >= len)) { - memcpy(buf, rBuf_+rPos_, len); - return true; + // If we have enough data, give a pointer to it, + // otherwise let the protcol use its slow path. + if (read_ && (rLen_-rPos_ >= *len)) { + *len = rLen_-rPos_; + return rBuf_+rPos_; } - return false; + return NULL; } void TFramedTransport::consume(uint32_t len) { @@ -293,15 +293,12 @@ void TMemoryBuffer::write(const uint8_t* buf, uint32_t len) { wPos_ += len; } -bool TMemoryBuffer::borrow(uint8_t* buf, uint32_t len) { - // Don't try to be clever with shifting buffers. - // If we have enough data, give it, otherwise - // let the protcol use its slow path. - if (wPos_-rPos_ >= len) { - memcpy(buf, buffer_ + rPos_, len); - return true; +const uint8_t* TMemoryBuffer::borrow(uint8_t* buf, uint32_t* len) { + if (wPos_-rPos_ >= *len) { + *len = wPos_-rPos_; + return buffer_ + rPos_; } - return false; + return NULL; } void TMemoryBuffer::consume(uint32_t len) { diff --git a/lib/cpp/src/transport/TTransportUtils.h b/lib/cpp/src/transport/TTransportUtils.h index 38d27871..ed64dec5 100644 --- a/lib/cpp/src/transport/TTransportUtils.h +++ b/lib/cpp/src/transport/TTransportUtils.h @@ -106,7 +106,7 @@ class TBufferedTransport : public TTransport { void flush(); - bool borrow(uint8_t* buf, uint32_t len); + const uint8_t* borrow(uint8_t* buf, uint32_t* len); void consume(uint32_t len); @@ -225,7 +225,7 @@ class TFramedTransport : public TTransport { void flush(); - bool borrow(uint8_t* buf, uint32_t len); + const uint8_t* borrow(uint8_t* buf, uint32_t* len); void consume(uint32_t len); @@ -424,7 +424,7 @@ class TMemoryBuffer : public TTransport { return wPos_ - rPos_; } - bool borrow(uint8_t* buf, uint32_t len); + const uint8_t* borrow(uint8_t* buf, uint32_t* len); void consume(uint32_t len); diff --git a/lib/cpp/src/transport/TZlibTransport.cpp b/lib/cpp/src/transport/TZlibTransport.cpp index 59e3a364..ea0b0a4b 100644 --- a/lib/cpp/src/transport/TZlibTransport.cpp +++ b/lib/cpp/src/transport/TZlibTransport.cpp @@ -234,15 +234,15 @@ void TZlibTransport::flushToZlib(const uint8_t* buf, int len, bool finish) { } } -bool TZlibTransport::borrow(uint8_t* buf, uint32_t len) { +const uint8_t* TZlibTransport::borrow(uint8_t* buf, uint32_t* len) { // Don't try to be clever with shifting buffers. - // If we have enough data, give it, otherwise - // let the protcol use its slow path. + // If we have enough data, give a pointer to it, + // otherwise let the protcol use its slow path. if (readAvail() >= (int)len) { - memcpy(buf, urbuf_ + urpos_, len); - return true; + *len = (uint32_t)readAvail(); + return urbuf_ + urpos_; } - return false; + return NULL; } void TZlibTransport::consume(uint32_t len) { diff --git a/lib/cpp/src/transport/TZlibTransport.h b/lib/cpp/src/transport/TZlibTransport.h index cc2522d9..dd8ae2f0 100644 --- a/lib/cpp/src/transport/TZlibTransport.h +++ b/lib/cpp/src/transport/TZlibTransport.h @@ -150,7 +150,7 @@ class TZlibTransport : public TTransport { void flush(); - bool borrow(uint8_t* buf, uint32_t len); + const uint8_t* borrow(uint8_t* buf, uint32_t* len); void consume(uint32_t len);