THRIFT-922. cpp: Convert transport classes to use non-virtual calls
Update the thrift transport classes to use non-virtual calls for most
functions. The correct implementation is determined at compile time via
templates now. Only the base TTransport class falls back to using
virtual function calls.
git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@1005134 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/lib/cpp/Makefile.am b/lib/cpp/Makefile.am
index 39382c8..f36ac82 100644
--- a/lib/cpp/Makefile.am
+++ b/lib/cpp/Makefile.am
@@ -130,6 +130,7 @@
src/transport/THttpServer.h \
src/transport/TSocket.h \
src/transport/TSocketPool.h \
+ src/transport/TVirtualTransport.h \
src/transport/TTransport.h \
src/transport/TTransportException.h \
src/transport/TTransportUtils.h \
diff --git a/lib/cpp/src/TLogging.h b/lib/cpp/src/TLogging.h
index 2df82dd..54cccbe 100644
--- a/lib/cpp/src/TLogging.h
+++ b/lib/cpp/src/TLogging.h
@@ -160,4 +160,26 @@
#define T_LOG_OPER(format_string,...)
#endif
+
+/**
+ * T_GLOBAL_DEBUG_VIRTUAL = 0: normal operation,
+ * virtual call debug messages disabled
+ * T_GLOBAL_DEBUG_VIRTUAL = 1: log a debug messages whenever an
+ * avoidable virtual call is made
+ */
+#define T_GLOBAL_DEBUG_VIRTUAL 0
+
+/**
+ * Log a message indicating that a virtual function call is being made.
+ *
+ * This should be disabled during normal use. It is intended to be used
+ * only to help debug serialization performance.
+ */
+#if T_GLOBAL_DEBUG_VIRTUAL > 0
+ #define T_VIRTUAL_CALL() \
+ fprintf(stderr,"[%s,%d] virtual call\n", __FILE__, __LINE__)
+#else
+ #define T_VIRTUAL_CALL()
+#endif
+
#endif // #ifndef _THRIFT_TLOGGING_H_
diff --git a/lib/cpp/src/transport/TBufferTransports.h b/lib/cpp/src/transport/TBufferTransports.h
index dbe7aca..a712775 100644
--- a/lib/cpp/src/transport/TBufferTransports.h
+++ b/lib/cpp/src/transport/TBufferTransports.h
@@ -24,6 +24,7 @@
#include "boost/scoped_array.hpp"
#include <transport/TTransport.h>
+#include <transport/TVirtualTransport.h>
#ifdef __GNUC__
#define TDB_LIKELY(val) (__builtin_expect((val), 1))
@@ -46,7 +47,7 @@
* that have to be done when the buffers are full or empty.
*
*/
-class TBufferBase : public TTransport {
+class TBufferBase : public TVirtualTransport<TBufferBase> {
public:
@@ -78,7 +79,7 @@
rBase_ = new_rBase;
return len;
}
- return facebook::thrift::transport::readAll(*this, buf, len);
+ return apache::thrift::transport::readAll(*this, buf, len);
}
/**
@@ -187,7 +188,8 @@
* stored to an in memory buffer before being written out.
*
*/
-class TBufferedTransport : public TBufferBase {
+class TBufferedTransport
+ : public TVirtualTransport<TBufferedTransport, TBufferBase> {
public:
static const int DEFAULT_BUFFER_SIZE = 512;
@@ -269,6 +271,12 @@
return transport_;
}
+ /*
+ * TVirtualTransport provides a default implementation of readAll().
+ * We want to use the TBufferBase version instead.
+ */
+ using TBufferBase::readAll;
+
protected:
void initPointers() {
setReadBuffer(rBuf_.get(), 0);
@@ -312,7 +320,8 @@
* other end to always do fixed-length reads.
*
*/
-class TFramedTransport : public TBufferBase {
+class TFramedTransport
+ : public TVirtualTransport<TFramedTransport, TBufferBase> {
public:
static const int DEFAULT_BUFFER_SIZE = 512;
@@ -371,6 +380,12 @@
return transport_;
}
+ /*
+ * TVirtualTransport provides a default implementation of readAll().
+ * We want to use the TBufferBase version instead.
+ */
+ using TBufferBase::readAll;
+
protected:
/**
* Reads a frame of input from the underlying stream.
@@ -423,7 +438,7 @@
* doubles as necessary. We've considered using scoped
*
*/
-class TMemoryBuffer : public TBufferBase {
+class TMemoryBuffer : public TVirtualTransport<TMemoryBuffer, TBufferBase> {
private:
// Common initialization done by all constructors.
@@ -666,6 +681,12 @@
// that had been provided by getWritePtr().
void wroteBytes(uint32_t len);
+ /*
+ * TVirtualTransport provides a default implementation of readAll().
+ * We want to use the TBufferBase version instead.
+ */
+ using TBufferBase::readAll;
+
protected:
void swap(TMemoryBuffer& that) {
using std::swap;
diff --git a/lib/cpp/src/transport/TFDTransport.h b/lib/cpp/src/transport/TFDTransport.h
index bda5d82..1d1a3e6 100644
--- a/lib/cpp/src/transport/TFDTransport.h
+++ b/lib/cpp/src/transport/TFDTransport.h
@@ -24,7 +24,7 @@
#include <sys/time.h>
#include "TTransport.h"
-#include "TServerSocket.h"
+#include "TVirtualTransport.h"
namespace apache { namespace thrift { namespace transport {
@@ -32,7 +32,7 @@
* Dead-simple wrapper around a file descriptor.
*
*/
-class TFDTransport : public TTransport {
+class TFDTransport : public TVirtualTransport<TFDTransport> {
public:
enum ClosePolicy
{ NO_CLOSE_ON_DESTROY = 0
diff --git a/lib/cpp/src/transport/TFileTransport.h b/lib/cpp/src/transport/TFileTransport.h
index 30d3a9b..f064b8e 100644
--- a/lib/cpp/src/transport/TFileTransport.h
+++ b/lib/cpp/src/transport/TFileTransport.h
@@ -273,6 +273,21 @@
return eofSleepTime_;
}
+ /*
+ * Override TTransport *_virt() functions to invoke our implementations.
+ * We cannot use TVirtualTransport to provide these, since we need to inherit
+ * virtually from TTransport.
+ */
+ virtual uint32_t read_virt(uint8_t* buf, uint32_t len) {
+ return this->read(buf, len);
+ }
+ virtual uint32_t readAll_virt(uint8_t* buf, uint32_t len) {
+ return this->readAll(buf, len);
+ }
+ virtual void write_virt(const uint8_t* buf, uint32_t len) {
+ this->write(buf, len);
+ }
+
private:
// helper functions for writing to a file
void enqueueEvent(const uint8_t* buf, uint32_t eventLen, bool blockUntilFlush);
diff --git a/lib/cpp/src/transport/THttpTransport.h b/lib/cpp/src/transport/THttpTransport.h
index cd58bcb..977c65f 100644
--- a/lib/cpp/src/transport/THttpTransport.h
+++ b/lib/cpp/src/transport/THttpTransport.h
@@ -21,6 +21,7 @@
#define _THRIFT_TRANSPORT_THTTPTRANSPORT_H_ 1
#include <transport/TBufferTransports.h>
+#include "TVirtualTransport.h"
namespace apache { namespace thrift { namespace transport {
@@ -31,7 +32,7 @@
* here is a VERY basic HTTP/1.1 client which supports HTTP 100 Continue,
* chunked transfer encoding, keepalive, etc. Tested against Apache.
*/
-class THttpTransport : public TTransport {
+class THttpTransport : public TVirtualTransport<THttpTransport> {
public:
THttpTransport(boost::shared_ptr<TTransport> transport);
diff --git a/lib/cpp/src/transport/TShortReadTransport.h b/lib/cpp/src/transport/TShortReadTransport.h
index 3df8a57..0d0eb86 100644
--- a/lib/cpp/src/transport/TShortReadTransport.h
+++ b/lib/cpp/src/transport/TShortReadTransport.h
@@ -23,6 +23,7 @@
#include <cstdlib>
#include <transport/TTransport.h>
+#include <transport/TVirtualTransport.h>
namespace apache { namespace thrift { namespace transport { namespace test {
@@ -32,7 +33,7 @@
* the read amount is randomly reduced before being passed through.
*
*/
-class TShortReadTransport : public TTransport {
+class TShortReadTransport : public TVirtualTransport<TShortReadTransport> {
public:
TShortReadTransport(boost::shared_ptr<TTransport> transport, double full_prob)
: transport_(transport)
diff --git a/lib/cpp/src/transport/TSocket.h b/lib/cpp/src/transport/TSocket.h
index f69a9a1..f195438 100644
--- a/lib/cpp/src/transport/TSocket.h
+++ b/lib/cpp/src/transport/TSocket.h
@@ -25,6 +25,7 @@
#include <netdb.h>
#include "TTransport.h"
+#include "TVirtualTransport.h"
#include "TServerSocket.h"
namespace apache { namespace thrift { namespace transport {
@@ -33,7 +34,7 @@
* TCP Socket implementation of the TTransport interface.
*
*/
-class TSocket : public TTransport {
+class TSocket : public TVirtualTransport<TSocket> {
/**
* We allow the TServerSocket acceptImpl() method to access the private
* members of a socket so that it can access the TSocket(int socket)
diff --git a/lib/cpp/src/transport/TTransport.h b/lib/cpp/src/transport/TTransport.h
index c453b8e..8f2bd3d 100644
--- a/lib/cpp/src/transport/TTransport.h
+++ b/lib/cpp/src/transport/TTransport.h
@@ -104,8 +104,13 @@
* @return How many bytes were actually read
* @throws TTransportException If an error occurs
*/
- virtual uint32_t read(uint8_t* /* buf */, uint32_t /* len */) {
- throw TTransportException(TTransportException::NOT_OPEN, "Base TTransport cannot read.");
+ uint32_t read(uint8_t* buf, uint32_t len) {
+ T_VIRTUAL_CALL();
+ return read_virt(buf, len);
+ }
+ virtual uint32_t read_virt(uint8_t* /* buf */, uint32_t /* len */) {
+ throw TTransportException(TTransportException::NOT_OPEN,
+ "Base TTransport cannot read.");
}
/**
@@ -116,7 +121,11 @@
* @return How many bytes read, which must be equal to size
* @throws TTransportException If insufficient data was read
*/
- virtual uint32_t readAll(uint8_t* buf, uint32_t len) {
+ uint32_t readAll(uint8_t* buf, uint32_t len) {
+ T_VIRTUAL_CALL();
+ return readAll_virt(buf, len);
+ }
+ virtual uint32_t readAll_virt(uint8_t* buf, uint32_t len) {
return apache::thrift::transport::readAll(*this, buf, len);
}
@@ -138,8 +147,13 @@
* @param buf The data to write out
* @throws TTransportException if an error occurs
*/
- virtual void write(const uint8_t* /* buf */, uint32_t /* len */) {
- throw TTransportException(TTransportException::NOT_OPEN, "Base TTransport cannot write.");
+ void write(const uint8_t* buf, uint32_t len) {
+ T_VIRTUAL_CALL();
+ write_virt(buf, len);
+ }
+ virtual void write_virt(const uint8_t* /* buf */, uint32_t /* len */) {
+ throw TTransportException(TTransportException::NOT_OPEN,
+ "Base TTransport cannot write.");
}
/**
@@ -191,7 +205,11 @@
* the transport's internal buffers.
* @throws TTransportException if an error occurs
*/
- virtual const uint8_t* borrow(uint8_t* /* buf */, uint32_t* /* len */) {
+ const uint8_t* borrow(uint8_t* buf, uint32_t* len) {
+ T_VIRTUAL_CALL();
+ return borrow_virt(buf, len);
+ }
+ virtual const uint8_t* borrow_virt(uint8_t* /* buf */, uint32_t* /* len */) {
return NULL;
}
@@ -204,8 +222,13 @@
* @param len How many bytes to consume
* @throws TTransportException If an error occurs
*/
- virtual void consume(uint32_t /* len */) {
- throw TTransportException(TTransportException::NOT_OPEN, "Base TTransport cannot consume.");
+ void consume(uint32_t len) {
+ T_VIRTUAL_CALL();
+ consume_virt(len);
+ }
+ virtual void consume_virt(uint32_t /* len */) {
+ throw TTransportException(TTransportException::NOT_OPEN,
+ "Base TTransport cannot consume.");
}
protected:
diff --git a/lib/cpp/src/transport/TTransportUtils.h b/lib/cpp/src/transport/TTransportUtils.h
index 8b0c076..dbb5c56 100644
--- a/lib/cpp/src/transport/TTransportUtils.h
+++ b/lib/cpp/src/transport/TTransportUtils.h
@@ -38,7 +38,7 @@
* go anywhere.
*
*/
-class TNullTransport : public TTransport {
+class TNullTransport : public TVirtualTransport<TNullTransport> {
public:
TNullTransport() {}
@@ -172,6 +172,18 @@
return dstTrans_;
}
+ /*
+ * Override TTransport *_virt() functions to invoke our implementations.
+ * We cannot use TVirtualTransport to provide these, since we need to inherit
+ * virtually from TTransport.
+ */
+ virtual uint32_t read_virt(uint8_t* buf, uint32_t len) {
+ return this->read(buf, len);
+ }
+ virtual void write_virt(const uint8_t* buf, uint32_t len) {
+ this->write(buf, len);
+ }
+
protected:
boost::shared_ptr<TTransport> srcTrans_;
boost::shared_ptr<TTransport> dstTrans_;
@@ -254,6 +266,21 @@
void seekToChunk(int32_t chunk);
void seekToEnd();
+ /*
+ * Override TTransport *_virt() functions to invoke our implementations.
+ * We cannot use TVirtualTransport to provide these, since we need to inherit
+ * virtually from TTransport.
+ */
+ virtual uint32_t read_virt(uint8_t* buf, uint32_t len) {
+ return this->read(buf, len);
+ }
+ virtual uint32_t readAll_virt(uint8_t* buf, uint32_t len) {
+ return this->readAll(buf, len);
+ }
+ virtual void write_virt(const uint8_t* buf, uint32_t len) {
+ this->write(buf, len);
+ }
+
protected:
// shouldn't be used
TPipedFileReaderTransport();
diff --git a/lib/cpp/src/transport/TVirtualTransport.h b/lib/cpp/src/transport/TVirtualTransport.h
new file mode 100644
index 0000000..1760681
--- /dev/null
+++ b/lib/cpp/src/transport/TVirtualTransport.h
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef _THRIFT_TRANSPORT_TVIRTUALTRANSPORT_H_
+#define _THRIFT_TRANSPORT_TVIRTUALTRANSPORT_H_ 1
+
+#include <transport/TTransport.h>
+
+namespace apache { namespace thrift { namespace transport {
+
+
+/**
+ * Helper class that provides default implementations of TTransport methods.
+ *
+ * This class provides default implementations of read(), readAll(), write(),
+ * borrow() and consume().
+ *
+ * In the TTransport base class, each of these methods simply invokes its
+ * virtual counterpart. This class overrides them to always perform the
+ * default behavior, without a virtual function call.
+ *
+ * The primary purpose of this class is to serve as a base class for
+ * TVirtualTransport, and prevent infinite recursion if one of its subclasses
+ * does not override the TTransport implementation of these methods. (Since
+ * TVirtualTransport::read_virt() calls read(), and TTransport::read() calls
+ * read_virt().)
+ */
+class TTransportDefaults : public TTransport {
+ public:
+ /*
+ * TTransport *_virt() methods provide reasonable default implementations.
+ * Invoke them non-virtually.
+ */
+ uint32_t read(uint8_t* buf, uint32_t len) {
+ return this->TTransport::read_virt(buf, len);
+ }
+ uint32_t readAll(uint8_t* buf, uint32_t len) {
+ return this->TTransport::readAll_virt(buf, len);
+ }
+ void write(const uint8_t* buf, uint32_t len) {
+ this->TTransport::write_virt(buf, len);
+ }
+ const uint8_t* borrow(uint8_t* buf, uint32_t* len) {
+ return this->TTransport::borrow_virt(buf, len);
+ }
+ void consume(uint32_t len) {
+ this->TTransport::consume_virt(len);
+ }
+
+ protected:
+ TTransportDefaults() {}
+};
+
+/**
+ * Helper class to provide polymorphism for subclasses of TTransport.
+ *
+ * This class implements *_virt() methods of TTransport, to call the
+ * non-virtual versions of these functions in the proper subclass.
+ *
+ * To define your own transport class using TVirtualTransport:
+ * 1) Derive your subclass from TVirtualTransport<your class>
+ * e.g: class MyTransport : public TVirtualTransport<MyTransport> {
+ * 2) Provide your own implementations of read(), readAll(), etc.
+ * These methods should be non-virtual.
+ *
+ * Transport implementations that need to use virtual inheritance when
+ * inheriting from TTransport cannot use TVirtualTransport.
+ *
+ * @author Chad Walters <chad@powerset.com>
+ */
+template <class Transport_, class Super_=TTransportDefaults>
+class TVirtualTransport : public Super_ {
+ public:
+ /*
+ * Implementations of the *_virt() functions, to call the subclass's
+ * non-virtual implementation function.
+ */
+ virtual uint32_t read_virt(uint8_t* buf, uint32_t len) {
+ return static_cast<Transport_*>(this)->read(buf, len);
+ }
+
+ virtual uint32_t readAll_virt(uint8_t* buf, uint32_t len) {
+ return static_cast<Transport_*>(this)->readAll(buf, len);
+ }
+
+ virtual void write_virt(const uint8_t* buf, uint32_t len) {
+ static_cast<Transport_*>(this)->write(buf, len);
+ }
+
+ virtual const uint8_t* borrow_virt(uint8_t* buf, uint32_t* len) {
+ return static_cast<Transport_*>(this)->borrow(buf, len);
+ }
+
+ virtual void consume_virt(uint32_t len) {
+ static_cast<Transport_*>(this)->consume(len);
+ }
+
+ /*
+ * Provide a default readAll() implementation that invokes
+ * read() non-virtually.
+ *
+ * Note: subclasses that use TVirtualTransport to derive from another
+ * transport implementation (i.e., not TTransportDefaults) should beware that
+ * this may override any non-default readAll() implementation provided by
+ * the parent transport class. They may need to redefine readAll() to call
+ * the correct parent implementation, if desired.
+ */
+ uint32_t readAll(uint8_t* buf, uint32_t len) {
+ Transport_* trans = static_cast<Transport_*>(this);
+ return ::apache::thrift::transport::readAll(*trans, buf, len);
+ }
+
+ protected:
+ TVirtualTransport() {}
+
+ /*
+ * Templatized constructors, to allow arguments to be passed to the Super_
+ * constructor. Currently we only support 0, 1, or 2 arguments, but
+ * additional versions can be added as needed.
+ */
+ template <typename Arg_>
+ TVirtualTransport(Arg_ const& arg) : Super_(arg) { }
+
+ template <typename Arg1_, typename Arg2_>
+ TVirtualTransport(Arg1_ const& a1, Arg2_ const& a2) : Super_(a1, a2) { }
+};
+
+}}} // apache::thrift::transport
+
+#endif // #ifndef _THRIFT_TRANSPORT_TVIRTUALTRANSPORT_H_
diff --git a/lib/cpp/src/transport/TZlibTransport.h b/lib/cpp/src/transport/TZlibTransport.h
index 1439d9d..61c43fe 100644
--- a/lib/cpp/src/transport/TZlibTransport.h
+++ b/lib/cpp/src/transport/TZlibTransport.h
@@ -22,6 +22,7 @@
#include <boost/lexical_cast.hpp>
#include <transport/TTransport.h>
+#include <transport/TVirtualTransport.h>
struct z_stream_s;
@@ -69,7 +70,7 @@
* the underlying transport is TBuffered or TMemory.
*
*/
-class TZlibTransport : public TTransport {
+class TZlibTransport : public TVirtualTransport<TZlibTransport> {
public:
/**