From e879c2f4db4bc0b7e5a0edfdf669c4fa477123a0 Mon Sep 17 00:00:00 2001 From: David Reiss Date: Wed, 6 Oct 2010 17:09:50 +0000 Subject: [PATCH] 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 --- lib/cpp/Makefile.am | 1 + lib/cpp/src/TLogging.h | 22 +++ lib/cpp/src/transport/TBufferTransports.h | 31 ++++- lib/cpp/src/transport/TFDTransport.h | 4 +- lib/cpp/src/transport/TFileTransport.h | 15 ++ lib/cpp/src/transport/THttpTransport.h | 3 +- lib/cpp/src/transport/TShortReadTransport.h | 3 +- lib/cpp/src/transport/TSocket.h | 3 +- lib/cpp/src/transport/TTransport.h | 39 ++++-- lib/cpp/src/transport/TTransportUtils.h | 29 +++- lib/cpp/src/transport/TVirtualTransport.h | 146 ++++++++++++++++++++ lib/cpp/src/transport/TZlibTransport.h | 3 +- 12 files changed, 279 insertions(+), 20 deletions(-) create mode 100644 lib/cpp/src/transport/TVirtualTransport.h diff --git a/lib/cpp/Makefile.am b/lib/cpp/Makefile.am index 39382c8e..f36ac821 100644 --- a/lib/cpp/Makefile.am +++ b/lib/cpp/Makefile.am @@ -130,6 +130,7 @@ include_transport_HEADERS = \ 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 2df82dd7..54cccbec 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 dbe7acae..a712775b 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 +#include #ifdef __GNUC__ #define TDB_LIKELY(val) (__builtin_expect((val), 1)) @@ -46,7 +47,7 @@ namespace apache { namespace thrift { namespace transport { * that have to be done when the buffers are full or empty. * */ -class TBufferBase : public TTransport { +class TBufferBase : public TVirtualTransport { public: @@ -78,7 +79,7 @@ class TBufferBase : public TTransport { 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 @@ class TBufferBase : public TTransport { * stored to an in memory buffer before being written out. * */ -class TBufferedTransport : public TBufferBase { +class TBufferedTransport + : public TVirtualTransport { public: static const int DEFAULT_BUFFER_SIZE = 512; @@ -269,6 +271,12 @@ class TBufferedTransport : public TBufferBase { 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 @@ class TBufferedTransportFactory : public TTransportFactory { * other end to always do fixed-length reads. * */ -class TFramedTransport : public TBufferBase { +class TFramedTransport + : public TVirtualTransport { public: static const int DEFAULT_BUFFER_SIZE = 512; @@ -371,6 +380,12 @@ class TFramedTransport : public TBufferBase { 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 @@ class TFramedTransportFactory : public TTransportFactory { * doubles as necessary. We've considered using scoped * */ -class TMemoryBuffer : public TBufferBase { +class TMemoryBuffer : public TVirtualTransport { private: // Common initialization done by all constructors. @@ -666,6 +681,12 @@ class TMemoryBuffer : public TBufferBase { // 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 bda5d82a..1d1a3e68 100644 --- a/lib/cpp/src/transport/TFDTransport.h +++ b/lib/cpp/src/transport/TFDTransport.h @@ -24,7 +24,7 @@ #include #include "TTransport.h" -#include "TServerSocket.h" +#include "TVirtualTransport.h" namespace apache { namespace thrift { namespace transport { @@ -32,7 +32,7 @@ namespace apache { namespace thrift { namespace transport { * Dead-simple wrapper around a file descriptor. * */ -class TFDTransport : public TTransport { +class TFDTransport : public TVirtualTransport { 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 30d3a9bd..f064b8eb 100644 --- a/lib/cpp/src/transport/TFileTransport.h +++ b/lib/cpp/src/transport/TFileTransport.h @@ -273,6 +273,21 @@ class TFileTransport : public TFileReaderTransport, 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 cd58bcb7..977c65fb 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 +#include "TVirtualTransport.h" namespace apache { namespace thrift { namespace transport { @@ -31,7 +32,7 @@ namespace apache { namespace thrift { namespace transport { * 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 { public: THttpTransport(boost::shared_ptr transport); diff --git a/lib/cpp/src/transport/TShortReadTransport.h b/lib/cpp/src/transport/TShortReadTransport.h index 3df8a57c..0d0eb861 100644 --- a/lib/cpp/src/transport/TShortReadTransport.h +++ b/lib/cpp/src/transport/TShortReadTransport.h @@ -23,6 +23,7 @@ #include #include +#include namespace apache { namespace thrift { namespace transport { namespace test { @@ -32,7 +33,7 @@ namespace apache { namespace thrift { namespace transport { namespace test { * the read amount is randomly reduced before being passed through. * */ -class TShortReadTransport : public TTransport { +class TShortReadTransport : public TVirtualTransport { public: TShortReadTransport(boost::shared_ptr transport, double full_prob) : transport_(transport) diff --git a/lib/cpp/src/transport/TSocket.h b/lib/cpp/src/transport/TSocket.h index f69a9a1a..f195438e 100644 --- a/lib/cpp/src/transport/TSocket.h +++ b/lib/cpp/src/transport/TSocket.h @@ -25,6 +25,7 @@ #include #include "TTransport.h" +#include "TVirtualTransport.h" #include "TServerSocket.h" namespace apache { namespace thrift { namespace transport { @@ -33,7 +34,7 @@ namespace apache { namespace thrift { namespace transport { * TCP Socket implementation of the TTransport interface. * */ -class TSocket : public TTransport { +class TSocket : public TVirtualTransport { /** * 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 c453b8e5..8f2bd3de 100644 --- a/lib/cpp/src/transport/TTransport.h +++ b/lib/cpp/src/transport/TTransport.h @@ -104,8 +104,13 @@ class TTransport { * @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 @@ class TTransport { * @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 @@ class TTransport { * @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 @@ class TTransport { * 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 @@ class TTransport { * @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 8b0c076f..dbb5c56f 100644 --- a/lib/cpp/src/transport/TTransportUtils.h +++ b/lib/cpp/src/transport/TTransportUtils.h @@ -38,7 +38,7 @@ namespace apache { namespace thrift { namespace transport { * go anywhere. * */ -class TNullTransport : public TTransport { +class TNullTransport : public TVirtualTransport { public: TNullTransport() {} @@ -172,6 +172,18 @@ class TPipedTransport : virtual public TTransport { 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 srcTrans_; boost::shared_ptr dstTrans_; @@ -254,6 +266,21 @@ class TPipedFileReaderTransport : public TPipedTransport, 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 00000000..17606819 --- /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 + +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 + * e.g: class MyTransport : public TVirtualTransport { + * 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 + */ +template +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(this)->read(buf, len); + } + + virtual uint32_t readAll_virt(uint8_t* buf, uint32_t len) { + return static_cast(this)->readAll(buf, len); + } + + virtual void write_virt(const uint8_t* buf, uint32_t len) { + static_cast(this)->write(buf, len); + } + + virtual const uint8_t* borrow_virt(uint8_t* buf, uint32_t* len) { + return static_cast(this)->borrow(buf, len); + } + + virtual void consume_virt(uint32_t len) { + static_cast(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(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 + TVirtualTransport(Arg_ const& arg) : Super_(arg) { } + + template + 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 1439d9de..61c43fe8 100644 --- a/lib/cpp/src/transport/TZlibTransport.h +++ b/lib/cpp/src/transport/TZlibTransport.h @@ -22,6 +22,7 @@ #include #include +#include struct z_stream_s; @@ -69,7 +70,7 @@ class TZlibTransportException : public TTransportException { * the underlying transport is TBuffered or TMemory. * */ -class TZlibTransport : public TTransport { +class TZlibTransport : public TVirtualTransport { public: /** -- 2.17.1