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
…ot path - ActivityListModel::data: cache FolderMan::instance() once per role evaluation instead of dereferencing the singleton twice for every row that has a file path. The Roles::PathRole and getDisplayPath lambda each used to call FolderMan::instance() twice; now once. - ActivityListModel::convertLinkToActionButton: move the reply icon URL construction inside the isReplyIconApplicable branch, and bake the trailing slash into the QStringLiteral so the path is a single immutable literal instead of two QString::operator+ allocations per activity link. - User::slotRefresh: cache _account.data() once and reuse instead of calling .data() five times in a row. Slight readability improvement alongside the avoided refcount manipulations. Signed-off-by: Qoole <2862661+qoole@users.noreply.github.com>
| 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
| 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; |
| { | ||
| 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
|
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.) |
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)