From 735346adb5caa8ee678ae0a8b91e8b43943a5bf1 Mon Sep 17 00:00:00 2001 From: Marco Bubke Date: Sun, 31 Jul 2022 01:09:08 +0200 Subject: [PATCH] Utils: SmallString trades copies for branches There can be quite some copies with the swap method for the default case that the string is inside the small string area. Instead now we minimize copies of the smallstring area at the expense of more branches. The swap approch needs no self tests and it uses the destructor to remove allocated resources but it can copy three times more often. And the destructor still has an additional branch in the copy case. Only in the move case we can use the destructor of the other instance. This can be surprising for some users which expect that the instance is empty after a move(actually assuming that a instance is empty after a move is undefined behavior). Because SmallString is designed to hold the string in almost all cases inside the small string area that case should be optimized. Change-Id: I22cb62aa99b9713a221f103971f149c05e9ff6fa Reviewed-by: Tim Jenssen Reviewed-by: Qt CI Bot --- src/libs/utils/smallstring.h | 33 +++++++++++++--------- tests/unit/unittest/smallstring-test.cpp | 35 ++++++++++++++++++++---- 2 files changed, 49 insertions(+), 19 deletions(-) diff --git a/src/libs/utils/smallstring.h b/src/libs/utils/smallstring.h index 1cdeea178be..60de005dbc8 100644 --- a/src/libs/utils/smallstring.h +++ b/src/libs/utils/smallstring.h @@ -170,19 +170,25 @@ public: Memory::deallocate(m_data.allocated.data.pointer); } - BasicSmallString(const BasicSmallString &string) + BasicSmallString(const BasicSmallString &other) { - if (string.isShortString() || string.isReadOnlyReference()) - m_data = string.m_data; + if (Q_LIKELY(other.isShortString() || other.isReadOnlyReference())) + m_data = other.m_data; else - new (this) BasicSmallString{string.data(), string.size()}; + new (this) BasicSmallString{other.data(), other.size()}; } BasicSmallString &operator=(const BasicSmallString &other) { - BasicSmallString copy = other; + if (Q_LIKELY(this != &other)) { + if (Q_UNLIKELY(hasAllocatedMemory())) + Memory::deallocate(m_data.allocated.data.pointer); - swap(*this, copy); + if (Q_LIKELY(other.isShortString() || other.isReadOnlyReference())) + m_data = other.m_data; + else + new (this) BasicSmallString{other.data(), other.size()}; + } return *this; } @@ -195,17 +201,18 @@ public: BasicSmallString &operator=(BasicSmallString &&other) noexcept { - swap(*this, other); + if (Q_LIKELY(this != &other)) { + if (Q_UNLIKELY(hasAllocatedMemory())) + Memory::deallocate(m_data.allocated.data.pointer); + + m_data = std::move(other.m_data); + other.m_data.reset(); + } return *this; } - BasicSmallString take() noexcept - { - auto tmp = std::move(*this); - - return tmp; - } + BasicSmallString take() noexcept { return std::move(*this); } BasicSmallString clone() const { BasicSmallString clonedString(m_data); diff --git a/tests/unit/unittest/smallstring-test.cpp b/tests/unit/unittest/smallstring-test.cpp index 0972a350b2d..4a287fe0ceb 100644 --- a/tests/unit/unittest/smallstring-test.cpp +++ b/tests/unit/unittest/smallstring-test.cpp @@ -1518,7 +1518,6 @@ TEST(SmallString, ShortSmallStringMoveConstuctor) auto copy = std::move(text); - ASSERT_TRUE(text.isEmpty()); ASSERT_THAT(copy, SmallString("text")); } @@ -1528,7 +1527,6 @@ TEST(SmallString, LongSmallStringMoveConstuctor) auto copy = std::move(text); - ASSERT_TRUE(text.isEmpty()); ASSERT_THAT(copy, SmallString("this is a very very very very long text")); } @@ -1538,7 +1536,6 @@ TEST(SmallString, ShortPathStringMoveConstuctor) auto copy = std::move(text); - ASSERT_TRUE(text.isEmpty()); ASSERT_THAT(copy, SmallString("text")); } @@ -1551,7 +1548,6 @@ TEST(SmallString, LongPathStringMoveConstuctor) auto copy = std::move(text); - ASSERT_TRUE(text.isEmpty()); ASSERT_THAT( copy, PathString( @@ -1642,7 +1638,6 @@ TEST(SmallString, ShortSmallStringMoveAssignment) copy = std::move(text); - ASSERT_THAT(text, SmallString("more text")); ASSERT_THAT(copy, SmallString("text")); } @@ -1653,10 +1648,38 @@ TEST(SmallString, LongSmallStringMoveAssignment) copy = std::move(text); - ASSERT_THAT(text, SmallString("more text")); ASSERT_THAT(copy, SmallString("this is a very very very very long text")); } +TEST(SmallString, ShortPathStringMoveAssignment) +{ + PathString text("text"); + PathString copy("more text"); + + copy = std::move(text); + + ASSERT_THAT(copy, SmallString("text")); +} + +TEST(SmallString, LongPathStringMoveAssignment) +{ + PathString text( + "this is a very very very very very very very very very very very very very very very very " + "very very very very very very very very very very very very very very very very very very " + "very very very very very very very very very very very very very very long text"); + PathString copy("more text"); + + copy = std::move(text); + + ASSERT_THAT( + copy, + PathString( + "this is a very very very very very very very very very very very very very very very " + "very very very very very very very very very very very very very very very very very " + "very very very very very very very very very very very very very very very very long " + "text")); +} + TEST(SmallString, ShortSmallStringTake) { SmallString text("text");