[WIP, RFC] A different approach to reducing KFileItem copies
Needs ReviewPublic

Authored by markg on Feb 4 2018, 3:05 AM.

Details

Reviewers
dfaure
Summary

Rationale; KFileItem is being created a _lot_ of times in Dolphin. Usually via a KFileItemList.
While KFileItem is COW, it is being made in hot code paths (think of updating actions, counting files/folders, etc)
so having as little of those as possible is a performance win.

For instance, with a "small" folder of just 5000 files i see a KFileItem copy constructer count of nearly 100.000 in callgrind.
The only actions i did:

  1. Open that folder
  2. Split view
  3. Opening an empty folder in one of the views (the empty folder is within the folder of 5000 entries for easy clicking)
  4. Selecting the 5000 items (minus the empty folder)
  5. Dragging the 5000 files to the empty other view
  6. Cancel the action

New functions:
KFileItemModel::execute - Just takes a lambda with an argument of: "const KFileItemModel::ItemData*". It iterates over all internal m_itemData entries.

KFileItemModel::executeOnIndexes - First argument is a single value container with int as value. Second is the same lambda type as the ::execute function.
This one iterates over the given indexes and grabs the ItemData object that belongs with it.

For now only DolphinView::statusBarText makes use of it. And all other functions that were calling calculateItemCount.

WIP and RFC part:
I do realize that i'm now just making internal data available in external functions. That might not be the most neat way, therefore RFC. I would very much like to know of a better way for this.
An alternative way would be to provide iterator support in KFileItemModel which would give even more flexibility but through a much neater API (and much more complicated to implement).
Please note that this really is a "optimization for massive folders" you will hardly notice it for smaller ones (say till 10.000 or so).
It's WIP because this is just a demonstration of how it could be done. This patch reduces KFileItem::KFileItem(KFileItem&) (aka, copy constructor) calls from ~100.000 to ~70.000.
I expect it can be reduced further to ~20.000 or so (with the above case), but i'm only going to try that if this approach is acceptable.

Diff Detail

Repository
R318 Dolphin
Branch
kfileitem_reduction
Lint
No Linters Available
Unit
No Unit Test Coverage
markg created this revision.Feb 4 2018, 3:05 AM
Restricted Application added a subscriber: Dolphin. · View Herald TranscriptFeb 4 2018, 3:05 AM
markg requested review of this revision.Feb 4 2018, 3:05 AM
markg updated this revision to Diff 26475.Feb 4 2018, 3:11 AM
  • Revert change to CMakeLists.txt
ngraham added a subscriber: ngraham.Feb 4 2018, 3:28 AM
dfaure added a comment.Feb 4 2018, 1:49 PM

When you measure the number of calls to the copy constructor, is that with a Debug build or a RelWithDebInfo build? The latter optimizes out a large number of calls to the copy ctor, so counting this on a debug build is a rather wrong measure.

src/views/dolphinview.cpp
1430

This could just be const KFileItem &item = to avoid copying, no ?
(though maybe there's one copy inside the method implementation)

markg added a comment.Feb 4 2018, 1:52 PM

When you measure the number of calls to the copy constructor, is that with a Debug build or a RelWithDebInfo build? The latter optimizes out a large number of calls to the copy ctor, so counting this on a debug build is a rather wrong measure.

The numbers are with RelWithDebInfo :)

markg added inline comments.Feb 4 2018, 2:13 PM
src/views/dolphinview.cpp
1430

Then we're heading into (N)RVO and copy elision details, that's tricky. The function signature is:
KFileItem fileItem(int index) const

It has 2 return paths, 1 for returning the KFileItem if found and one for returning an empty one if nothing was found. There was something with multiple return paths, but i can't quite find what it was.
Also, copy elision talks about the same function return type (it's "KFileItem") as in the request side (which would be const KFileItem &) and is therefore not the same. Thus a copy is made.

This is a big assumption on my side. I'm not that familiar with these standard rules so i could very well be completely wrong.
So, ehh.. I don't know for sure but my guess is still a copy.