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 e79d4f1..55f3902 100644
--- a/lib/cpp/src/protocol/TDenseProtocol.cpp
+++ b/lib/cpp/src/protocol/TDenseProtocol.cpp
@@ -180,12 +180,13 @@
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 b3d8820..4e3c218 100644
--- a/lib/cpp/src/transport/TTransport.h
+++ b/lib/cpp/src/transport/TTransport.h
@@ -139,21 +139,31 @@
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 b10958c..b5e7fa8 100644
--- a/lib/cpp/src/transport/TTransportUtils.cpp
+++ b/lib/cpp/src/transport/TTransportUtils.cpp
@@ -59,15 +59,15 @@
}
}
-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 @@
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 @@
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 38d2787..ed64dec 100644
--- a/lib/cpp/src/transport/TTransportUtils.h
+++ b/lib/cpp/src/transport/TTransportUtils.h
@@ -106,7 +106,7 @@
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 @@
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 @@
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 59e3a36..ea0b0a4 100644
--- a/lib/cpp/src/transport/TZlibTransport.cpp
+++ b/lib/cpp/src/transport/TZlibTransport.cpp
@@ -234,15 +234,15 @@
}
}
-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 cc2522d..dd8ae2f 100644
--- a/lib/cpp/src/transport/TZlibTransport.h
+++ b/lib/cpp/src/transport/TZlibTransport.h
@@ -150,7 +150,7 @@
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);