From: Adam Simpkins Date: Wed, 20 Nov 2013 04:29:25 +0000 (-0800) Subject: support stack-allocated IOBufs X-Git-Tag: v0.22.0~729 X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;ds=sidebyside;h=7e31a318f06c83a3fcc5b49cb69e139b1d3b650f;p=folly.git support stack-allocated IOBufs Summary: Previously, all IOBuf APIs required that IOBufs always be allocated on the heap. The only methods provided to create IOBufs returned unique_ptr. This adds new methods to support creating IOBufs on the stack. This is useful in cases where the IOBuf will be short-lived, and the overhead of the heap allocation is undesirable. (One use case is to wrap an existing buffer in a short-lived IOBuf so that it can be used with the Cursor API.) I have currently made IOBufs movable but not copyable. (Move operations clearly should move only a single IOBuf, but it is not clear if the copy operators should copy only a single IOBuf or the entire chain.) Test Plan: Updated the unit tests to test the new CREATE, WRAP_BUFFER, TAKE_OWNERSHIP, and COPY_BUFFER constructors, as well as the move constructor and assignment operator. Reviewed By: davejwatson@fb.com FB internal diff: D1067341 --- diff --git a/folly/io/IOBuf.cpp b/folly/io/IOBuf.cpp index 923cbb22..950e6293 100644 --- a/folly/io/IOBuf.cpp +++ b/folly/io/IOBuf.cpp @@ -18,8 +18,11 @@ #include "folly/io/IOBuf.h" -#include "folly/Malloc.h" +#include "folly/Conv.h" #include "folly/Likely.h" +#include "folly/Malloc.h" +#include "folly/Memory.h" +#include "folly/ScopeGuard.h" #include #include @@ -51,8 +54,29 @@ enum : uint32_t { kDefaultCombinedBufSize = 1024 }; +// Helper function for IOBuf::takeOwnership() +void takeOwnershipError(bool freeOnError, void* buf, + folly::IOBuf::FreeFunction freeFn, + void* userData) { + if (!freeOnError) { + return; + } + if (!freeFn) { + free(buf); + return; + } + try { + freeFn(buf, userData); + } catch (...) { + // The user's free function is not allowed to throw. + // (We are already in the middle of throwing an exception, so + // we cannot let this exception go unhandled.) + abort(); + } } +} // unnamed namespace + namespace folly { struct IOBuf::HeapPrefix { @@ -160,6 +184,30 @@ void IOBuf::freeInternalBuf(void* buf, void* userData) { releaseStorage(storage, kDataInUse); } +IOBuf::IOBuf(CreateOp, uint32_t capacity) + : next_(this), + prev_(this), + data_(nullptr), + length_(0), + flags_(0), + type_(kExtAllocated) { + allocExtBuffer(capacity, &buf_, &sharedInfo_, &capacity_); + data_ = buf_; +} + +IOBuf::IOBuf(CopyBufferOp op, const void* buf, uint32_t size, + uint32_t headroom, uint32_t minTailroom) + : IOBuf(CREATE, headroom + size + minTailroom) { + advance(headroom); + memcpy(writableData(), buf, size); + append(size); +} + +IOBuf::IOBuf(CopyBufferOp op, ByteRange br, + uint32_t headroom, uint32_t minTailroom) + : IOBuf(op, br.data(), br.size(), headroom, minTailroom) { +} + unique_ptr IOBuf::create(uint32_t capacity) { // For smaller-sized buffers, allocate the IOBuf, SharedInfo, and the buffer // all with a single allocation. @@ -196,22 +244,7 @@ unique_ptr IOBuf::createCombined(uint32_t capacity) { } unique_ptr IOBuf::createSeparate(uint32_t capacity) { - // Allocate an external buffer - uint8_t* buf; - SharedInfo* sharedInfo; - uint32_t actualCapacity; - allocExtBuffer(capacity, &buf, &sharedInfo, &actualCapacity); - - // Allocate the IOBuf header - try { - return unique_ptr(new IOBuf(kExtAllocated, 0, - buf, actualCapacity, - buf, 0, - sharedInfo)); - } catch (...) { - free(buf); - throw; - } + return make_unique(CREATE, capacity); } unique_ptr IOBuf::createChain( @@ -230,52 +263,71 @@ unique_ptr IOBuf::createChain( return out; } +IOBuf::IOBuf(TakeOwnershipOp, void* buf, uint32_t capacity, uint32_t length, + FreeFunction freeFn, void* userData, + bool freeOnError) + : next_(this), + prev_(this), + data_(static_cast(buf)), + buf_(static_cast(buf)), + length_(length), + capacity_(capacity), + flags_(kFlagFreeSharedInfo), + type_(kExtUserSupplied) { + try { + sharedInfo_ = new SharedInfo(freeFn, userData); + } catch (...) { + takeOwnershipError(freeOnError, buf, freeFn, userData); + throw; + } +} + unique_ptr IOBuf::takeOwnership(void* buf, uint32_t capacity, uint32_t length, FreeFunction freeFn, void* userData, bool freeOnError) { - SharedInfo* sharedInfo = NULL; try { // TODO: We could allocate the IOBuf object and SharedInfo all in a single // memory allocation. We could use the existing HeapStorage class, and // define a new kSharedInfoInUse flag. We could change our code to call // releaseStorage(kFlagFreeSharedInfo) when this kFlagFreeSharedInfo, // rather than directly calling delete. - sharedInfo = new SharedInfo(freeFn, userData); - - uint8_t* bufPtr = static_cast(buf); - return unique_ptr(new IOBuf(kExtUserSupplied, kFlagFreeSharedInfo, - bufPtr, capacity, - bufPtr, length, - sharedInfo)); + // + // Note that we always pass freeOnError as false to the constructor. + // If the constructor throws we'll handle it below. (We have to handle + // allocation failures from make_unique too.) + return make_unique(TAKE_OWNERSHIP, buf, capacity, length, + freeFn, userData, false); } catch (...) { - delete sharedInfo; - if (freeOnError) { - if (freeFn) { - try { - freeFn(buf, userData); - } catch (...) { - // The user's free function is not allowed to throw. - abort(); - } - } else { - free(buf); - } - } + takeOwnershipError(freeOnError, buf, freeFn, userData); throw; } } +IOBuf::IOBuf(WrapBufferOp, const void* buf, uint32_t capacity) + : IOBuf(kExtUserOwned, kFlagUserOwned, + // We cast away the const-ness of the buffer here. + // This is okay since IOBuf users must use unshare() to create a copy + // of this buffer before writing to the buffer. + static_cast(const_cast(buf)), capacity, + static_cast(const_cast(buf)), capacity, + nullptr) { +} + +IOBuf::IOBuf(WrapBufferOp op, ByteRange br) + : IOBuf(op, br.data(), folly::to(br.size())) { +} + unique_ptr IOBuf::wrapBuffer(const void* buf, uint32_t capacity) { - // We cast away the const-ness of the buffer here. - // This is okay since IOBuf users must use unshare() to create a copy of - // this buffer before writing to the buffer. - uint8_t* bufPtr = static_cast(const_cast(buf)); - return unique_ptr(new IOBuf(kExtUserSupplied, kFlagUserOwned, - bufPtr, capacity, - bufPtr, capacity, - NULL)); + return make_unique(WRAP_BUFFER, buf, capacity); +} + +IOBuf::IOBuf() noexcept { +} + +IOBuf::IOBuf(IOBuf&& other) noexcept { + *this = std::move(other); } IOBuf::IOBuf(ExtBufTypeEnum type, @@ -313,6 +365,54 @@ IOBuf::~IOBuf() { decrementRefcount(); } +IOBuf& IOBuf::operator=(IOBuf&& other) noexcept { + // If we are part of a chain, delete the rest of the chain. + while (next_ != this) { + // Since unlink() returns unique_ptr() and we don't store it, + // it will automatically delete the unlinked element. + (void)next_->unlink(); + } + + // Decrement our refcount on the current buffer + decrementRefcount(); + + // Take ownership of the other buffer's data + data_ = other.data_; + buf_ = other.buf_; + length_ = other.length_; + capacity_ = other.capacity_; + flags_ = other.flags_; + type_ = other.type_; + sharedInfo_ = other.sharedInfo_; + // Reset other so it is a clean state to be destroyed. + other.data_ = nullptr; + other.buf_ = nullptr; + other.length_ = 0; + other.capacity_ = 0; + other.flags_ = kFlagUserOwned; + other.type_ = kExtUserOwned; + other.sharedInfo_ = nullptr; + + // If other was part of the chain, assume ownership of the rest of its chain. + // (It's only valid to perform move assignment on the head of a chain.) + if (other.next_ != &other) { + next_ = other.next_; + next_->prev_ = this; + other.next_ = &other; + + prev_ = other.prev_; + prev_->next_ = this; + other.prev_ = &other; + } + + // Sanity check to make sure that other is in a valid state to be destroyed. + DCHECK_EQ(other.prev_, &other); + DCHECK_EQ(other.next_, &other); + DCHECK(other.flags_ & kFlagUserOwned); + + return *this; +} + bool IOBuf::empty() const { const IOBuf* current = this; do { diff --git a/folly/io/IOBuf.h b/folly/io/IOBuf.h index 72ef2f7f..43fddc7d 100644 --- a/folly/io/IOBuf.h +++ b/folly/io/IOBuf.h @@ -165,27 +165,53 @@ namespace folly { * accessed by multiple threads. * * - * IOBuf Object Allocation/Sharing - * ------------------------------- + * IOBuf Object Allocation + * ----------------------- * - * IOBuf objects themselves are always allocated on the heap. The IOBuf - * constructors are private, so IOBuf objects may not be created on the stack. - * In part this is done since some IOBuf objects use small-buffer optimization - * and contain the buffer data immediately after the IOBuf object itself. The - * coalesce() and unshare() methods also expect to be able to delete subsequent - * IOBuf objects in the chain if they are no longer needed due to coalescing. + * IOBuf objects themselves exist separately from the data buffer they point + * to. Therefore one must also consider how to allocate and manage the IOBuf + * objects. * - * The IOBuf structure also does not provide room for an intrusive refcount on - * the IOBuf object itself, only the underlying data buffer is reference - * counted. If users want to share the same IOBuf object between multiple - * parts of the code, they are responsible for managing this sharing on their - * own. (For example, by using a shared_ptr. Alternatively, users always have - * the option of using clone() to create a second IOBuf that points to the same - * underlying buffer.) + * It is more common to allocate IOBuf objects on the heap, using the create(), + * takeOwnership(), or wrapBuffer() factory functions. The clone()/cloneOne() + * functions also return new heap-allocated IOBufs. The createCombined() + * function allocates the IOBuf object and data storage space together, in a + * single memory allocation. This can improve performance, particularly if you + * know that the data buffer and the IOBuf itself will have similar lifetimes. * - * With jemalloc, allocating small objects like IOBuf objects should be - * relatively fast, and the cost of allocating IOBuf objects on the heap and - * cloning new IOBufs should be relatively cheap. + * That said, it is also possible to allocate IOBufs on the stack or inline + * inside another object as well. This is useful for cases where the IOBuf is + * short-lived, or when the overhead of allocating the IOBuf on the heap is + * undesirable. + * + * However, note that stack-allocated IOBufs may only be used as the head of a + * chain (or standalone as the only IOBuf in a chain). All non-head members of + * an IOBuf chain must be heap allocated. (All functions to add nodes to a + * chain require a std::unique_ptr, which enforces this requrement.) + * + * Additionally, no copy-constructor or assignment operator currently exists, + * so stack-allocated IOBufs may only be moved, not copied. (Technically + * nothing is preventing us from adding a copy constructor and assignment + * operator. However, it seems like this would add the possibility for some + * confusion. We would need to determine if these functions would copy just a + * single buffer, or the entire chain.) + * + * + * IOBuf Sharing + * ------------- + * + * The IOBuf class manages sharing of the underlying buffer that it points to, + * maintaining a reference count if multiple IOBufs are pointing at the same + * buffer. + * + * However, it is the callers responsibility to manage sharing and ownership of + * IOBuf objects themselves. The IOBuf structure does not provide room for an + * intrusive refcount on the IOBuf object itself, only the underlying data + * buffer is reference counted. If users want to share the same IOBuf object + * between multiple parts of the code, they are responsible for managing this + * sharing on their own. (For example, by using a shared_ptr. Alternatively, + * users always have the option of using clone() to create a second IOBuf that + * points to the same underlying buffer.) */ namespace detail { // Is T a unique_ptr<> to a standard-layout type? @@ -202,6 +228,11 @@ class IOBuf { public: class Iterator; + enum CreateOp { CREATE }; + enum WrapBufferOp { WRAP_BUFFER }; + enum TakeOwnershipOp { TAKE_OWNERSHIP }; + enum CopyBufferOp { COPY_BUFFER }; + typedef ByteRange value_type; typedef Iterator iterator; typedef Iterator const_iterator; @@ -221,6 +252,7 @@ class IOBuf { * Throws std::bad_alloc on error. */ static std::unique_ptr create(uint32_t capacity); + IOBuf(CreateOp, uint32_t capacity); /** * Create a new IOBuf, using a single memory allocation to allocate space @@ -275,18 +307,25 @@ class IOBuf { * default) the buffer will be freed before throwing the error. */ static std::unique_ptr takeOwnership(void* buf, uint32_t capacity, - FreeFunction freeFn = NULL, - void* userData = NULL, + FreeFunction freeFn = nullptr, + void* userData = nullptr, bool freeOnError = true) { return takeOwnership(buf, capacity, capacity, freeFn, userData, freeOnError); } + IOBuf(TakeOwnershipOp op, void* buf, uint32_t capacity, + FreeFunction freeFn = nullptr, void* userData = nullptr, + bool freeOnError = true) + : IOBuf(op, buf, capacity, capacity, freeFn, userData, freeOnError) {} static std::unique_ptr takeOwnership(void* buf, uint32_t capacity, uint32_t length, - FreeFunction freeFn = NULL, - void* userData = NULL, + FreeFunction freeFn = nullptr, + void* userData = nullptr, bool freeOnError = true); + IOBuf(TakeOwnershipOp, void* buf, uint32_t capacity, uint32_t length, + FreeFunction freeFn = nullptr, void* userData = nullptr, + bool freeOnError = true); /** * Create a new IOBuf pointing to an existing data buffer made up of @@ -338,6 +377,8 @@ class IOBuf { CHECK_LE(br.size(), std::numeric_limits::max()); return wrapBuffer(br.data(), br.size()); } + IOBuf(WrapBufferOp op, const void* buf, uint32_t capacity); + IOBuf(WrapBufferOp op, ByteRange br); /** * Convenience function to create a new IOBuf object that copies data from a @@ -353,6 +394,10 @@ class IOBuf { CHECK_LE(br.size(), std::numeric_limits::max()); return copyBuffer(br.data(), br.size(), headroom, minTailroom); } + IOBuf(CopyBufferOp op, const void* buf, uint32_t size, + uint32_t headroom=0, uint32_t minTailroom=0); + IOBuf(CopyBufferOp op, ByteRange br, + uint32_t headroom=0, uint32_t minTailroom=0); /** * Convenience function to create a new IOBuf object that copies data from a @@ -368,6 +413,9 @@ class IOBuf { static std::unique_ptr copyBuffer(const std::string& buf, uint32_t headroom=0, uint32_t minTailroom=0); + IOBuf(CopyBufferOp op, const std::string& buf, + uint32_t headroom=0, uint32_t minTailroom=0) + : IOBuf(op, buf.data(), buf.size(), headroom, minTailroom) {} /** * A version of copyBuffer() that returns a null pointer if the input string @@ -779,7 +827,7 @@ class IOBuf { prev_->next_ = next_; prev_ = this; next_ = this; - return std::unique_ptr((next == this) ? NULL : next); + return std::unique_ptr((next == this) ? nullptr : next); } /** @@ -1016,6 +1064,39 @@ class IOBuf { Iterator begin() const; Iterator end() const; + /** + * Allocate a new null buffer. + * + * This can be used to allocate an empty IOBuf on the stack. It will have no + * space allocated for it. This is generally useful only to later use move + * assignment to fill out the IOBuf. + */ + IOBuf() noexcept; + + /** + * Move constructor and assignment operator. + * + * In general, you should only ever move the head of an IOBuf chain. + * Internal nodes in an IOBuf chain are owned by the head of the chain, and + * should not be moved from. (Technically, nothing prevents you from moving + * a non-head node, but the moved-to node will replace the moved-from node in + * the chain. This has implications for ownership, since non-head nodes are + * owned by the chain head. You are then responsible for relinquishing + * ownership of the moved-to node, and manually deleting the moved-from + * node.) + * + * With the move assignment operator, the destination of the move should be + * the head of an IOBuf chain or a solitary IOBuf not part of a chain. If + * the move destination is part of a chain, all other IOBufs in the chain + * will be deleted. + * + * (We currently don't provide a copy constructor or assignment operator. + * The main reason is because it is not clear these operations should copy + * the entire chain or just the single IOBuf.) + */ + IOBuf(IOBuf&& other) noexcept; + IOBuf& operator=(IOBuf&& other) noexcept; + private: enum FlagsEnum : uint32_t { kFlagUserOwned = 0x1, @@ -1039,7 +1120,7 @@ class IOBuf { SharedInfo(FreeFunction fn, void* arg); // A pointer to a function to call to free the buffer when the refcount - // hits 0. If this is NULL, free() will be used instead. + // hits 0. If this is null, free() will be used instead. FreeFunction freeFn; void* userData; std::atomic refcount; @@ -1102,11 +1183,11 @@ class IOBuf { * Links to the next and the previous IOBuf in this chain. * * The chain is circularly linked (the last element in the chain points back - * at the head), and next_ and prev_ can never be NULL. If this IOBuf is the + * at the head), and next_ and prev_ can never be null. If this IOBuf is the * only element in the chain, next_ and prev_ will both point to this. */ - IOBuf* next_; - IOBuf* prev_; + IOBuf* next_{this}; + IOBuf* prev_{this}; /* * A pointer to the start of the data referenced by this IOBuf, and the @@ -1114,15 +1195,15 @@ class IOBuf { * * This may refer to any subsection of the actual buffer capacity. */ - uint8_t* data_; - uint8_t* buf_; - uint32_t length_; - uint32_t capacity_; - mutable uint32_t flags_; - uint32_t type_; + uint8_t* data_{nullptr}; + uint8_t* buf_{nullptr}; + uint32_t length_{0}; + uint32_t capacity_{0}; + mutable uint32_t flags_{kFlagUserOwned}; + uint32_t type_{kExtUserOwned}; // SharedInfo may be NULL if kFlagUserOwned is set. It is non-NULL // in all other cases. - SharedInfo* sharedInfo_; + SharedInfo* sharedInfo_{nullptr}; struct DeleterBase { virtual ~DeleterBase() { } diff --git a/folly/io/test/IOBufTest.cpp b/folly/io/test/IOBufTest.cpp index baab1c85..ce443f40 100644 --- a/folly/io/test/IOBufTest.cpp +++ b/folly/io/test/IOBufTest.cpp @@ -142,7 +142,26 @@ TEST(IOBuf, TakeOwnership) { iobuf3.reset(); EXPECT_EQ(1, deleteCount); - + deleteCount = 0; + { + uint32_t size4 = 1234; + uint8_t *buf4 = new uint8_t[size4]; + uint32_t length4 = 48; + IOBuf iobuf4(IOBuf::TAKE_OWNERSHIP, buf4, size4, length4, + deleteArrayBuffer, &deleteCount); + EXPECT_EQ(buf4, iobuf4.data()); + EXPECT_EQ(length4, iobuf4.length()); + EXPECT_EQ(buf4, iobuf4.buffer()); + EXPECT_EQ(size4, iobuf4.capacity()); + + IOBuf iobuf5 = std::move(iobuf4); + EXPECT_EQ(buf4, iobuf5.data()); + EXPECT_EQ(length4, iobuf5.length()); + EXPECT_EQ(buf4, iobuf5.buffer()); + EXPECT_EQ(size4, iobuf5.capacity()); + EXPECT_EQ(0, deleteCount); + } + EXPECT_EQ(1, deleteCount); } TEST(IOBuf, WrapBuffer) { @@ -161,6 +180,14 @@ TEST(IOBuf, WrapBuffer) { EXPECT_EQ(size2, iobuf2->length()); EXPECT_EQ(buf2.get(), iobuf2->buffer()); EXPECT_EQ(size2, iobuf2->capacity()); + + uint32_t size3 = 4321; + unique_ptr buf3(new uint8_t[size3]); + IOBuf iobuf3(IOBuf::WRAP_BUFFER, buf3.get(), size3); + EXPECT_EQ(buf3.get(), iobuf3.data()); + EXPECT_EQ(size3, iobuf3.length()); + EXPECT_EQ(buf3.get(), iobuf3.buffer()); + EXPECT_EQ(size3, iobuf3.capacity()); } TEST(IOBuf, CreateCombined) { @@ -660,6 +687,13 @@ TEST(IOBuf, copyBuffer) { EXPECT_EQ(3, buf->headroom()); EXPECT_EQ(0, buf->length()); EXPECT_LE(6, buf->tailroom()); + + // A stack-allocated version + IOBuf stackBuf(IOBuf::COPY_BUFFER, s, 1, 2); + EXPECT_EQ(1, stackBuf.headroom()); + EXPECT_EQ(s, std::string(reinterpret_cast(stackBuf.data()), + stackBuf.length())); + EXPECT_LE(2, stackBuf.tailroom()); } TEST(IOBuf, maybeCopyBuffer) { @@ -931,6 +965,47 @@ TEST(IOBuf, getIov) { EXPECT_EQ(count - 3, iov.size()); } +TEST(IOBuf, move) { + // Default allocate an IOBuf on the stack + IOBuf outerBuf; + char data[] = "foobar"; + uint32_t length = sizeof(data); + uint32_t actualCapacity{0}; + const void* ptr{nullptr}; + + { + // Create a small IOBuf on the stack. + // Note that IOBufs created on the stack always use an external buffer. + IOBuf b1(IOBuf::CREATE, 10); + actualCapacity = b1.capacity(); + EXPECT_GE(actualCapacity, 10); + EXPECT_EQ(0, b1.length()); + EXPECT_FALSE(b1.isShared()); + ptr = b1.data(); + ASSERT_TRUE(ptr != nullptr); + memcpy(b1.writableTail(), data, length); + b1.append(length); + EXPECT_EQ(length, b1.length()); + + // Use the move constructor + IOBuf b2(std::move(b1)); + EXPECT_EQ(ptr, b2.data()); + EXPECT_EQ(length, b2.length()); + EXPECT_EQ(actualCapacity, b2.capacity()); + EXPECT_FALSE(b2.isShared()); + + // Use the move assignment operator + outerBuf = std::move(b2); + // Close scope, destroying b1 and b2 + // (which are both be invalid now anyway after moving out of them) + } + + EXPECT_EQ(ptr, outerBuf.data()); + EXPECT_EQ(length, outerBuf.length()); + EXPECT_EQ(actualCapacity, outerBuf.capacity()); + EXPECT_FALSE(outerBuf.isShared()); +} + int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); google::ParseCommandLineFlags(&argc, &argv, true);