perf(activity): cache singleton pointers and avoid string concat in hot path#9997
perf(activity): cache singleton pointers and avoid string concat in hot path#9997qoole wants to merge 1 commit into
Conversation
| auto *folderMan = FolderMan::instance(); | ||
| const auto folder = folderMan->folder(activity._folder); |
There was a problem hiding this comment.
why not make FolderMan::instance() an inline function if we think this is overused and may lead to overhead ?
would not increase code complexity
There was a problem hiding this comment.
Done in 8a10051 — made FolderMan::instance() an inline accessor in the header instead of caching the pointer locally. Removes the call overhead everywhere without the local-variable change.
| if (isReplyIconApplicable) { | ||
| activityLinkCopy._imageSource = QString(replyButtonPath + "/"); | ||
| activityLinkCopy._imageSourceHovered = QString(replyButtonPath + "/"); | ||
| const QString replyButtonPath = QStringLiteral("image://svgimage-custom-color/reply.svg/"); |
There was a problem hiding this comment.
would be much better to use a string literal to generate a proper QString
| const QString replyButtonPath = QStringLiteral("image://svgimage-custom-color/reply.svg/"); | |
| using namespace Qt::StringLiterals; | |
| const auto replyButtonPath = u"image://svgimage-custom-color/reply.svg/"_s; |
There was a problem hiding this comment.
Applied your suggestion in 8a10051 (u"..."_s string literal).
| { | ||
| slotRefreshUserStatus(); | ||
|
|
||
| auto *account = _account.data(); |
There was a problem hiding this comment.
AccountPtr::data() is an inline method
your changes will not bring any difference from performance point of view
please revert the changes around _account.data() use
There was a problem hiding this comment.
Reverted in 8a10051 — _account.data() usage in usermodel.cpp is back to the original.
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
qoole
force-pushed
the
perf/activity-list-allocations
branch
from
95ab3f7 to
702b65c
Compare
…in hot path - FolderMan::instance(): make it an inline accessor returning the static pointer, removing the out-of-line call overhead at every use site without adding any local caching or extra complexity. - ActivityListModel::convertLinkToActionButton: move the reply icon URL construction inside the isReplyIconApplicable branch and bake the trailing slash into the string literal so the path is a single immutable QString instead of two QString::operator+ allocations. Signed-off-by: Qoole <[email protected]>
qoole
force-pushed
the
perf/activity-list-allocations
branch
from
702b65c to
8a10051
Compare
|
Addressed in 8a10051:
|
2 participants
Summary
Three small allocation/dereference cleanups in the activity list and user model. Each one is per-row or per-refresh, and the activity list
data()runs on every visible row of every model update.ActivityListModel::data— cacheFolderMan::instance()Two places in
data()dereference theFolderMansingleton twice in succession:Cached the pointer once per role evaluation. Same behavior, half the singleton lookups.
ActivityListModel::convertLinkToActionButton— reply icon URLTwo improvements bundled:
replyButtonPathwas constructed unconditionally even whenisReplyIconApplicableis false. Moved inside the branch.QStringLiteral(".../reply.svg") + "/", allocating a QString concat every call. Baked the trailing slash into the literal so it's a single immutableQStringLiteral.Same URL on the wire; one allocation removed per applicable activity link.
User::slotRefresh— cache_account.data()slotRefreshwas calling_account.data()five times in a row. Cached once into a localauto *account. Behavior identical (the smart pointer is already strong-rooted onthis->_accountfor the duration of the function).Checklist
AI (if applicable)