From cc6324665eb61eca136e9057ce1c72a2a0c32d31 Mon Sep 17 00:00:00 2001 From: Ahmad Samir Date: Wed, 14 Dec 2022 23:53:57 +0200 Subject: [PATCH] QString: don't detach in insert() Instead of detaching when the string is shared, or if the the insertion would cause a reallocation, create a new string and copy characters to it as needed, then swap it with "this" string. This is more efficient than detaching which would copy the whole string before inserting, as some characters would be copied multiple times. Use detachAndGrow(), otherwise QStringBuilder unitests fail: PASS : tst_QStringBuilder1::initTestCase() FAIL! : tst_QStringBuilder1::scenario() 'prepends < max_prepends' returned FALSE. () Loc: [tests/auto/corelib/text/qstringbuilder/qstringbuilder1/stringbuilder.cpp(61)] PASS : tst_QStringBuilder1::cleanupTestCase() The issue is that now when inserting, if the string is going to reallocated, we create a new string, so the freeSpaceAtBegin() optimization doesn't work the same way. void checkItWorksWithFreeSpaceAtBegin(const String &chunk, const Separator &separator) { // GIVEN: a String with freeSpaceAtBegin() and less than chunk.size() freeSpaceAtEnd() String str; int prepends = 0; const int max_prepends = 10; while (str.data_ptr().freeSpaceAtBegin() < chunk.size() && prepends++ < max_prepends) str.prepend(chunk); QVERIFY(prepends < max_prepends); ... ... each str.prepend() would have reallocated. [ChangeLog][QtCore][QString] Calling insert() on a currently shared string is now done more efficiently. Task-number: QTBUG-106186 Change-Id: I07ce8d6bde50919fdc587433e624ace9cee05be8 Reviewed-by: Thiago Macieira --- src/corelib/text/qstring.cpp | 17 ++ .../auto/corelib/text/qstring/tst_qstring.cpp | 173 ++++++++++++++---- 2 files changed, 150 insertions(+), 40 deletions(-) diff --git a/src/corelib/text/qstring.cpp b/src/corelib/text/qstring.cpp index 305d6e9a67..daa8f8d98b 100644 --- a/src/corelib/text/qstring.cpp +++ b/src/corelib/text/qstring.cpp @@ -2951,6 +2951,23 @@ static void insert_helper(QString &str, qsizetype i, T toInsert) const qsizetype insert_size = toInsert.size(); const qsizetype newSize = str_d.size + difference + insert_size; const auto side = i == 0 ? QArrayData::GrowsAtBeginning : QArrayData::GrowsAtEnd; + + if (str_d.needsDetach() || needsReallocate(str, newSize)) { + const auto cbegin = str.cbegin(); + const auto cend = str.cend(); + const auto insert_start = difference == 0 ? std::next(cbegin, i) : cend; + QString other; + // Using detachAndGrow() so that prepend optimization works and QStringBuilder + // unittests pass + other.data_ptr().detachAndGrow(side, newSize, nullptr, nullptr); + other.append(QStringView(cbegin, insert_start)); + other.resize(i, u' '); + other.append(toInsert); + other.append(QStringView(insert_start, cend)); + str.swap(other); + return; + } + str_d.detachAndGrow(side, difference + insert_size, nullptr, nullptr); Q_CHECK_PTR(str_d.data()); str.resize(newSize); diff --git a/tests/auto/corelib/text/qstring/tst_qstring.cpp b/tests/auto/corelib/text/qstring/tst_qstring.cpp index 2dfa9e34df..ac3d4d0f85 100644 --- a/tests/auto/corelib/text/qstring/tst_qstring.cpp +++ b/tests/auto/corelib/text/qstring/tst_qstring.cpp @@ -282,11 +282,22 @@ static void do_apply1(MemFun mf) QFETCH(A1, a1); QFETCH(QString, expected); - Arg(arg).apply1(s, mf, a1); + // Test when the string is shared + QString str = s; + Arg(arg).apply1(str, mf, a1); - QCOMPARE(s, expected); - QCOMPARE(s.isEmpty(), expected.isEmpty()); - QCOMPARE(s.isNull(), expected.isNull()); + QCOMPARE(str, expected); + QCOMPARE(str.isEmpty(), expected.isEmpty()); + QCOMPARE(str.isNull(), expected.isNull()); + + // Test when the string is not shared + str = s; + str.detach(); + Arg(arg).apply1(str, mf, a1); + QCOMPARE(str, expected); + QCOMPARE(str.isEmpty(), expected.isEmpty()); + // A detached string is not null + // QCOMPARE(str.isNull(), expected.isNull()); } class tst_QString : public QObject @@ -2870,43 +2881,127 @@ void tst_QString::insert_data(DataOptions options) void tst_QString::insert_special_cases() { QString a; + QString dummy_share; - a = "Ys"; - QCOMPARE(a.insert(1,'e'), QString("Yes")); - QCOMPARE(a.insert(3,'!'), QString("Yes!")); - QCOMPARE(a.insert(5,'?'), QString("Yes! ?")); - QCOMPARE(a.insert(-1,'a'), QString("Yes! a?")); + { + // Test when string is not shared + a = "Ys"; + QCOMPARE(a.insert(1,'e'), QString("Yes")); + QCOMPARE(a.insert(3,'!'), QString("Yes!")); + QCOMPARE(a.insert(5,'?'), QString("Yes! ?")); + QCOMPARE(a.insert(-1,'a'), QString("Yes! a?")); + } + { + // Test when string is shared + a = "Ys"; + dummy_share = a; + QCOMPARE(a.insert(1,'e'), QString("Yes")); + dummy_share = a; + QCOMPARE(a.insert(3,'!'), QString("Yes!")); + dummy_share = a; + QCOMPARE(a.insert(5,'?'), QString("Yes! ?")); + dummy_share = a; + QCOMPARE(a.insert(-1,'a'), QString("Yes! a?")); + } a = "ABC"; - QCOMPARE(a.insert(5,"DEF"), QString("ABC DEF")); + dummy_share = a; + QCOMPARE(dummy_share.insert(5,"DEF"), QString("ABC DEF")); // Shared + QCOMPARE(a.insert(5,"DEF"), QString("ABC DEF")); // Not shared after dummy_shared.insert() - a = "ABC"; - QCOMPARE(a.insert(2, QString()), QString("ABC")); - QCOMPARE(a.insert(0,"ABC"), QString("ABCABC")); - QCOMPARE(a, QString("ABCABC")); - QCOMPARE(a.insert(0,a), QString("ABCABCABCABC")); + { + // Test when string is not shared + a = "ABC"; + QCOMPARE(a.insert(2, QString()), QString("ABC")); + QCOMPARE(a.insert(0,"ABC"), QString("ABCABC")); + QCOMPARE(a, QString("ABCABC")); + QCOMPARE(a.insert(0,a), QString("ABCABCABCABC")); - QCOMPARE(a, QString("ABCABCABCABC")); - QCOMPARE(a.insert(0,'<'), QString("'), QString("<>ABCABCABCABC")); + QCOMPARE(a, QString("ABCABCABCABC")); + QCOMPARE(a.insert(0,'<'), QString("'), QString("<>ABCABCABCABC")); + } + { + // Test when string is shared + a = "ABC"; + dummy_share = a; + QCOMPARE(a.insert(2, QString()), QString("ABC")); + dummy_share = a; + QCOMPARE(a.insert(0,"ABC"), QString("ABCABC")); + dummy_share = a; + QCOMPARE(a, QString("ABCABC")); + dummy_share = a; + QCOMPARE(a.insert(0,a), QString("ABCABCABCABC")); + + QCOMPARE(a, QString("ABCABCABCABC")); + dummy_share = a; + QCOMPARE(a.insert(0,'<'), QString("'), QString("<>ABCABCABCABC")); + } - a = "Meal"; const QString montreal = QStringLiteral("Montreal"); - QCOMPARE(a.insert(1, QLatin1String("ontr")), montreal); - QCOMPARE(a.insert(4, ""), montreal); - QCOMPARE(a.insert(3, QLatin1String("")), montreal); - QCOMPARE(a.insert(3, QLatin1String(nullptr)), montreal); - QCOMPARE(a.insert(3, static_cast(0)), montreal); - QCOMPARE(a.insert(0, QLatin1String("a")), QLatin1String("aMontreal")); + { + // Test when string is not shared + a = "Meal"; + QCOMPARE(a.insert(1, QLatin1String("ontr")), montreal); + QCOMPARE(a.insert(4, ""), montreal); + QCOMPARE(a.insert(3, QLatin1String("")), montreal); + QCOMPARE(a.insert(3, QLatin1String(nullptr)), montreal); + QCOMPARE(a.insert(3, static_cast(0)), montreal); + QCOMPARE(a.insert(0, QLatin1String("a")), QLatin1String("aMontreal")); + } + { + // Test when string is shared + a = "Meal"; + dummy_share = a; + QCOMPARE(a.insert(1, QLatin1String("ontr")), montreal); + dummy_share = a; + QCOMPARE(a.insert(4, ""), montreal); + dummy_share = a; + QCOMPARE(a.insert(3, QLatin1String("")), montreal); + dummy_share = a; + QCOMPARE(a.insert(3, QLatin1String(nullptr)), montreal); + dummy_share = a; + QCOMPARE(a.insert(3, static_cast(0)), montreal); + dummy_share = a; + QCOMPARE(a.insert(0, QLatin1String("a")), QLatin1String("aMontreal")); + } - a = "Mont"; - QCOMPARE(a.insert(a.size(), QLatin1String("real")), montreal); - QCOMPARE(a.insert(a.size() + 1, QLatin1String("ABC")), QString("Montreal ABC")); + { + // Test when string is not shared + a = "Mont"; + QCOMPARE(a.insert(a.size(), QLatin1String("real")), montreal); + QCOMPARE(a.insert(a.size() + 1, QLatin1String("ABC")), QString("Montreal ABC")); + } + { + // Test when string is shared + a = "Mont"; + dummy_share = a; + QCOMPARE(a.insert(a.size(), QLatin1String("real")), montreal); + dummy_share = a; + QCOMPARE(a.insert(a.size() + 1, QLatin1String("ABC")), QString("Montreal ABC")); + } + + { + // Test when string is not shared + a = "AEF"; + QCOMPARE(a.insert(1, QLatin1String("BCD")), QString("ABCDEF")); + QCOMPARE(a.insert(3, QLatin1String("-")), QString("ABC-DEF")); + QCOMPARE(a.insert(a.size() + 1, QLatin1String("XYZ")), QString("ABC-DEF XYZ")); + } + + { + // Test when string is shared + a = "AEF"; + dummy_share = a ; + QCOMPARE(a.insert(1, QLatin1String("BCD")), QString("ABCDEF")); + dummy_share = a ; + QCOMPARE(a.insert(3, QLatin1String("-")), QString("ABC-DEF")); + dummy_share = a ; + QCOMPARE(a.insert(a.size() + 1, QLatin1String("XYZ")), QString("ABC-DEF XYZ")); + } - a = "AEF"; - QCOMPARE(a.insert(1, QLatin1String("BCD")), QString("ABCDEF")); - QCOMPARE(a.insert(3, QLatin1String("-")), QString("ABC-DEF")); - QCOMPARE(a.insert(a.size() + 1, QLatin1String("XYZ")), QString("ABC-DEF XYZ")); { a = "one"; @@ -2914,6 +3009,12 @@ void tst_QString::insert_special_cases() QString b(a.data_ptr()->freeSpaceAtEnd(), u'b'); QCOMPARE(a.insert(a.size() + 1, QLatin1String(b.toLatin1())), QString("aone ") + b); } + { + a = "one"; + a.prepend(u'a'); + QString b(a.data_ptr()->freeSpaceAtEnd(), u'b'); + QCOMPARE(a.insert(a.size() + 1, b), QString("aone ") + b); + } { a = "onetwothree"; @@ -2922,14 +3023,6 @@ void tst_QString::insert_special_cases() QString b(a.data_ptr()->freeSpaceAtEnd() + 1, u'b'); QCOMPARE(a.insert(a.size() + 1, QLatin1String(b.toLatin1())), QString("e ") + b); } - - { - a = "one"; - a.prepend(u'a'); - QString b(a.data_ptr()->freeSpaceAtEnd(), u'b'); - QCOMPARE(a.insert(a.size() + 1, b), QString("aone ") + b); - } - { a = "onetwothree"; while (a.size() - 1)