Skip to content

qt: Optimize string concatenation by default#313

Merged
laanwj merged 2 commits intobitcoin-core:masterfrom
hebasto:210503-builder
May 26, 2021
Merged

qt: Optimize string concatenation by default#313
laanwj merged 2 commits intobitcoin-core:masterfrom
hebasto:210503-builder

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented May 3, 2021

From Qt docs:

... multiple uses of the [QString] '+' operator usually means multiple memory allocations. When concatenating n substrings, where n > 2, there can be as many as n - 1 calls to the memory allocator.

With this PR

... the '+' will automatically be performed as the QStringBuilder '%' everywhere.

The change in the src/Makefile.qt.include file does not justify submitting this PR into the main repo, IMHO.

@hebasto hebasto force-pushed the 210503-builder branch 2 times, most recently from daa63ed to b4e1480 Compare May 3, 2021 10:46
jarolrod
jarolrod reviewed May 5, 2021
View reviewed changes
Copy link
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b4e1480 🥃

This simplifies and reduces the mental burden of having to think about "am I concatenating a string in the most efficient way possible?"

I could not find any other occurrences of including QStringBuilder or occurrences of concatenating string with %.

@hebasto
Copy link
Member Author

hebasto commented May 5, 2021

@Talkless

Could you look into this PR?

@jarolrod jarolrod mentioned this pull request May 5, 2021
Merged
@Talkless
Copy link

Talkless commented May 5, 2021

It's true that QtCreator, and Qt itself is built with that flag, but we might want to discuss if we want that "implicit magic". There is some caveat with this kind of code:

#include <QString>
#include <QDebug>

QString zoo() {
    return QString{"zoo"};
}

int main()
{
    const auto foo = QString("bar") % QLatin1Char(' ') % zoo();
    qDebug() << foo; // crash because QStringBuilder<T...> expression template will have references to dead temporaries
}

"maybe" it is possible to write similar code and "sometimes" not crash, IDK. Not sure if this is dangerous in any new code, i.e. if it will be missed by reviewers, if it will pass without "sometimes" crashing, etc. Although, we are talking about GUI code, so.. not sure.

I personally still use % for "explicitness", but as @jarolrod said, it removes burned of thinking about inefficient concatenation & stuff... And less annoying nit comments from me :D .

Talkless
Talkless reviewed May 5, 2021
View reviewed changes
Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

src/qt/peertablemodel.cpp
case Address:
// prepend to peer address down-arrow symbol for inbound connection and up-arrow for outbound connection
return QString(rec->nodeStats.fInbound ? "↓ " : "↑ ") + QString::fromStdString(rec->nodeStats.addrName);
return QString::fromStdString((rec->nodeStats.fInbound ? "↓ " : "↑ ") + rec->nodeStats.addrName);
Copy link

@Talkless Talkless May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is motive for this change?

Copy link
Member Author

@hebasto hebasto May 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

qt/peertablemodel.cpp:117:20: error: no viable conversion from returned value of type 'QStringBuilder<typename QConcatenable<QString>::type, typename QConcatenable<QString>::type>' (aka 'QStringBuilder<QString, QString>') to function return type 'QVariant'
            return QString(rec->nodeStats.fInbound ? "↓ " : "↑ ") + QString::fromStdString(rec->nodeStats.addrName);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@hebasto hebasto changed the title qt, build: Optimize string concatenation by default Optimize string concatenation by default May 9, 2021
@hebasto
Copy link
Member Author

hebasto commented May 15, 2021

@Talkless

It's true that QtCreator, and Qt itself is built with that flag, but we might want to discuss if we want that "implicit magic".

You are right! Discussion is a good mean to reach (rough) consensus about suggested changes.

There is some caveat with this kind of code:

You code snippet could be fixed by s/auto/QString/, and such caveats could be documented in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code.

hebasto added 2 commits May 15, 2021 19:10
@hebasto
qt, build: Optimize string concatenation
3fd3a0f 
The defined QT_USE_QSTRINGBUILDER macro means using the QStringBuilder
for efficient string concatenation in all Qt code by default.
@hebasto
qt, refactor: Revert explicit including QStringBuilder
a02c970
@hebasto
Copy link
Member Author

hebasto commented May 15, 2021

Updated b4e1480 -> a02c970 (pr313.02 -> pr313.03, diff):

@Talkless
Copy link

Talkless commented May 19, 2021

You code snippet could be fixed by s/auto/QString/

Yes, but that's the "danger", as one needs to know these kinda caveats. Yes, docs could help.

@hebasto hebasto requested a review from maflcko May 19, 2021 18:30
Talkless
Talkless approved these changes May 19, 2021
View reviewed changes
Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK a02c970, built successfully on Debian Sid with Qt 5.15.2, but did not check if any displayed strings are "wrong" after refactoring.

jarolrod
jarolrod reviewed May 26, 2021
View reviewed changes
Copy link
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a02c970

Tested most of the strings on Qt 5.15.2 macOS 11.3

@laanwj
Copy link
Member

laanwj commented May 26, 2021

Code review ACK a02c970

This simplifies and reduces the mental burden of having to think about "am I concatenating a string in the most efficient way possible?"

I don't think there is any place in bitcoin where concatenating Qt strings is actually a bottleneck, but agree with this perspective.

It's true that QtCreator, and Qt itself is built with that flag,

That's reassuring also in the sense that it probably won't just go away.

@laanwj laanwj changed the title Optimize string concatenation by default qt: Optimize string concatenation by default May 26, 2021
@laanwj laanwj merged commit 456c8d6 into bitcoin-core:master May 26, 2021
@hebasto hebasto deleted the 210503-builder branch May 26, 2021 13:23
@jonatack
Copy link
Member

jonatack commented May 26, 2021

Approach ACK, this looks like a good idea indeed.

@jarolrod jarolrod mentioned this pull request May 26, 2021
Merged
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 27, 2021
@laanwj
Merge bitcoin-core/gui#313: qt: Optimize string concatenation by default
dffb9c4
@ryanofsky ryanofsky mentioned this pull request Jun 2, 2021
Merged
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge 456c8d6 into merged_master (Bitcoin PR bitcoin-core/gui#313)
7db44ed
@luke-jr luke-jr mentioned this pull request Jul 5, 2022
Closed
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@maflcko maflcko Awaiting requested review from maflcko

2 more reviewers

@jarolrod jarolrod jarolrod left review comments

@Talkless Talkless Talkless approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hebasto @Talkless @laanwj @jonatack @jarolrod