From 1c523f26df01d9f6c877a89fd330f2e4a1a69e5a Mon Sep 17 00:00:00 2001 From: Pavlo Kushnir Date: Wed, 4 May 2016 00:27:53 -0700 Subject: [PATCH] Optimize copy/move Summary: this diff provides methods that allow to avoid temporaries while cloning IOBufs. Also optimizes move constructor a bit - there is no need to call `decrementRefCount`, check for self assignment, etc. Reviewed By: yfeldblum Differential Revision: D3250456 fb-gh-sync-id: 32b0180c28f36151c6786dba6f511b491b224200 fbshipit-source-id: 32b0180c28f36151c6786dba6f511b491b224200 --- folly/io/IOBuf.cpp | 64 +++++++++++++++-------- folly/io/IOBuf.h | 20 ++++++- folly/io/test/IOBufBenchmark.cpp | 90 ++++++++++++++++++++++++++++++++ folly/io/test/IOBufTest.cpp | 47 +++++++++++++++++ 4 files changed, 198 insertions(+), 23 deletions(-) create mode 100644 folly/io/test/IOBufBenchmark.cpp diff --git a/folly/io/IOBuf.cpp b/folly/io/IOBuf.cpp index 5a505588..c3b22221 100644 --- a/folly/io/IOBuf.cpp +++ b/folly/io/IOBuf.cpp @@ -335,12 +335,38 @@ unique_ptr IOBuf::wrapBuffer(const void* buf, uint64_t capacity) { IOBuf::IOBuf() noexcept { } -IOBuf::IOBuf(IOBuf&& other) noexcept { - *this = std::move(other); +IOBuf::IOBuf(IOBuf&& other) noexcept + : data_(other.data_), + buf_(other.buf_), + length_(other.length_), + capacity_(other.capacity_), + flagsAndSharedInfo_(other.flagsAndSharedInfo_) { + // Reset other so it is a clean state to be destroyed. + other.data_ = nullptr; + other.buf_ = nullptr; + other.length_ = 0; + other.capacity_ = 0; + other.flagsAndSharedInfo_ = 0; + + // 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); } IOBuf::IOBuf(const IOBuf& other) { - other.cloneInto(*this); + *this = other.cloneAsValue(); } IOBuf::IOBuf(InternalConstructor, @@ -473,39 +499,35 @@ void IOBuf::prependChain(unique_ptr&& iobuf) { } unique_ptr IOBuf::clone() const { - unique_ptr ret = make_unique(); - cloneInto(*ret); - return ret; + return make_unique(cloneAsValue()); } unique_ptr IOBuf::cloneOne() const { - unique_ptr ret = make_unique(); - cloneOneInto(*ret); - return ret; + return make_unique(cloneOneAsValue()); } -void IOBuf::cloneInto(IOBuf& other) const { - IOBuf tmp; - cloneOneInto(tmp); +IOBuf IOBuf::cloneAsValue() const { + auto tmp = cloneOneAsValue(); for (IOBuf* current = next_; current != this; current = current->next_) { tmp.prependChain(current->cloneOne()); } - other = std::move(tmp); + return tmp; } -void IOBuf::cloneOneInto(IOBuf& other) const { - SharedInfo* info = sharedInfo(); - if (info) { +IOBuf IOBuf::cloneOneAsValue() const { + if (SharedInfo* info = sharedInfo()) { setFlags(kFlagMaybeShared); - } - other = IOBuf(InternalConstructor(), - flagsAndSharedInfo_, buf_, capacity_, - data_, length_); - if (info) { info->refcount.fetch_add(1, std::memory_order_acq_rel); } + return IOBuf( + InternalConstructor(), + flagsAndSharedInfo_, + buf_, + capacity_, + data_, + length_); } void IOBuf::unshareOneSlow() { diff --git a/folly/io/IOBuf.h b/folly/io/IOBuf.h index f6a43d66..32ce6e0d 100644 --- a/folly/io/IOBuf.h +++ b/folly/io/IOBuf.h @@ -1065,6 +1065,12 @@ class IOBuf { */ std::unique_ptr clone() const; + /** + * Similar to clone(). But returns IOBuf by value rather than heap-allocating + * it. + */ + IOBuf cloneAsValue() const; + /** * Return a new IOBuf with the same data as this IOBuf. * @@ -1073,17 +1079,27 @@ class IOBuf { */ std::unique_ptr cloneOne() const; + /** + * Similar to cloneOne(). But returns IOBuf by value rather than + * heap-allocating it. + */ + IOBuf cloneOneAsValue() const; + /** * Similar to Clone(). But use other as the head node. Other nodes in the * chain (if any) will be allocted on heap. */ - void cloneInto(IOBuf& other) const; + void cloneInto(IOBuf& other) const { + other = cloneAsValue(); + } /** * Similar to CloneOne(). But to fill an existing IOBuf instead of a new * IOBuf. */ - void cloneOneInto(IOBuf& other) const; + void cloneOneInto(IOBuf& other) const { + other = cloneOneAsValue(); + } /** * Return an iovector suitable for e.g. writev() diff --git a/folly/io/test/IOBufBenchmark.cpp b/folly/io/test/IOBufBenchmark.cpp new file mode 100644 index 00000000..68efa2ba --- /dev/null +++ b/folly/io/test/IOBufBenchmark.cpp @@ -0,0 +1,90 @@ +/* + * Copyright 2016 Facebook, Inc. + * + * Licensed 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. + */ + +#include +#include + +using folly::IOBuf; + +BENCHMARK(cloneOneBenchmark, iters) { + IOBuf buf(IOBuf::CREATE, 10); + while (iters--) { + auto copy = buf.cloneOne(); + folly::doNotOptimizeAway(copy->capacity()); + } +} + +BENCHMARK(cloneOneIntoBenchmark, iters) { + IOBuf buf(IOBuf::CREATE, 10); + IOBuf copy; + while (iters--) { + buf.cloneOneInto(copy); + folly::doNotOptimizeAway(copy.capacity()); + } +} + +BENCHMARK(cloneBenchmark, iters) { + IOBuf buf(IOBuf::CREATE, 10); + while (iters--) { + auto copy = buf.clone(); + folly::doNotOptimizeAway(copy->capacity()); + } +} + +BENCHMARK(cloneIntoBenchmark, iters) { + IOBuf buf(IOBuf::CREATE, 10); + IOBuf copy; + while (iters--) { + buf.cloneInto(copy); + folly::doNotOptimizeAway(copy.capacity()); + } +} + +BENCHMARK(moveBenchmark, iters) { + IOBuf buf(IOBuf::CREATE, 10); + while (iters--) { + auto tmp = std::move(buf); + folly::doNotOptimizeAway(tmp.capacity()); + buf = std::move(tmp); + } +} + +BENCHMARK(copyBenchmark, iters) { + IOBuf buf(IOBuf::CREATE, 10); + while (iters--) { + auto copy = buf; + folly::doNotOptimizeAway(copy.capacity()); + } +} + +/** + * ============================================================================ + * folly/io/test/IOBufBenchmark.cpp relative time/iter iters/s + * ============================================================================ + * cloneOneBenchmark 49.03ns 20.39M + * cloneOneIntoBenchmark 26.36ns 37.93M + * cloneBenchmark 49.43ns 20.23M + * cloneIntoBenchmark 30.03ns 33.30M + * moveBenchmark 15.35ns 65.14M + * copyBenchmark 33.63ns 29.73M + * ============================================================================ +*/ + +int main(int argc, char** argv) { + gflags::ParseCommandLineFlags(&argc, &argv, true); + folly::runBenchmarks(); + return 0; +} diff --git a/folly/io/test/IOBufTest.cpp b/folly/io/test/IOBufTest.cpp index ca39013d..7267b49c 100644 --- a/folly/io/test/IOBufTest.cpp +++ b/folly/io/test/IOBufTest.cpp @@ -1149,6 +1149,53 @@ TEST(IOBuf, CopyConstructorAndAssignmentOperator) { EXPECT_FALSE(buf->isShared()); } +TEST(IOBuf, CloneAsValue) { + auto buf = IOBuf::create(4096); + append(buf, "hello world"); + { + auto buf2 = IOBuf::create(4096); + append(buf2, " goodbye"); + buf->prependChain(std::move(buf2)); + EXPECT_FALSE(buf->isShared()); + } + + { + auto copy = buf->cloneOneAsValue(); + EXPECT_TRUE(buf->isShared()); + EXPECT_TRUE(copy.isShared()); + EXPECT_EQ((void*)buf->data(), (void*)copy.data()); + EXPECT_TRUE(buf->isChained()); + EXPECT_FALSE(copy.isChained()); + + auto copy2 = buf->cloneAsValue(); + EXPECT_TRUE(buf->isShared()); + EXPECT_TRUE(copy.isShared()); + EXPECT_TRUE(copy2.isShared()); + EXPECT_TRUE(buf->isChained()); + EXPECT_TRUE(copy2.isChained()); + + copy.unshareOne(); + EXPECT_TRUE(buf->isShared()); + EXPECT_FALSE(copy.isShared()); + EXPECT_NE((void*)buf->data(), (void*)copy.data()); + EXPECT_TRUE(copy2.isShared()); + + auto p = reinterpret_cast(copy.data()); + EXPECT_EQ("hello world", std::string(p, copy.length())); + + copy2.coalesce(); + EXPECT_FALSE(buf->isShared()); + EXPECT_FALSE(copy.isShared()); + EXPECT_FALSE(copy2.isShared()); + EXPECT_FALSE(copy2.isChained()); + + auto p2 = reinterpret_cast(copy2.data()); + EXPECT_EQ("hello world goodbye", std::string(p2, copy2.length())); + } + + EXPECT_FALSE(buf->isShared()); +} + namespace { // Use with string literals only std::unique_ptr wrap(const char* str) { -- 2.34.1