KDirModelV2, KDirListerV2 and UDSEntryV2 suggestions

[David Faure]

On Friday 28 December 2012 02:50:53 Mark wrote:
> Hi,
> 
> -- Be sure to read all of this (long!) mail or parts won't make sense! --
> -- cross posting to frameworks and KCD since it concerns both --

I'll let the k-c-d people come here if they want to participate.

> cases that would be oke, but if you have really massive folders then
> having a list of - let say - 100.000 files/folders in KFileItem
> objects begins to be quite taxing for the CPU. That alone isn't an
> issue, but the fact that you won't even see those 100.000 items in any
> view makes it quite wasteful to even store it. So what i want to do is
> store a big (much smaller in memory) UDSEntry list where KFileItem
> objects are "lazy loaded" when actually needed. That would be a lot
> less taxing for the CPU and memory. 

CPU-wise, we could do the lazy initialization inside KFileItem itself, and the 
result would be pretty much the same, without exposing everyone to rather 
internal UDSEntries.
Memory-wise you're right, though, KFileItem adds quite a bunch of data. Maybe 
this could be changed so that it stores its data directly into the UDSEntry, 
even when created from a local path directly (no UDSEntry as input).

> Also the caching in
> KDirListerCache would very likely be a lot smaller when using a
> UDSEntryList. 

You'll end up adding tons of methods to UDSEntry, to make this possible, so 
basically you'll turn UDSEntry into KFileItem, API wise. This doesn't make 
sense to me, better make KFileItem slimmer internally, and keep the API.

UDSEntry is the raw storage [for things that come from a kioslave], KFileItem 
is the API on top [plus support for things that don't come from a kioslave].

Better keep this separated (e.g. UDSEntry is enough on the slave side), and fix 
the "memory usage" of KFileItem by changing its implementation.

> --- KDirModelV2 API ---- (on top of the functions that have to be
> re-implemented)
> - void setDirLister( KDirLister* dirLister );
> - KDirLister* dirLister() const;
> - KFileItem itemForIndex( const QModelIndex& index ) const;
> - void setAdditionalRoles(const QHash<int, QByteArray> & roleNames)
> - The column names should get some defaults that can also be used in QML.

This can be done in the current KDirModel, no reason to "fork" it just for 
this.

> - All the required functions for drag/drop support should be
> implemented though that doesn't matter much anymore when used in QML..

We *could* do that (my comment in the code says "not sure", I'm still not 
sure) -- but it would kill any hopes for core/gui separation, since a user-
initiated copy operation can trigger conflict dialogs etc. And different apps 
might want different things to happen on drop (e.g. think of kdevelop, where 
"drop" might mean "add to project", not "copy here"). But I'm open to being 
convinced of a useful default implementation if you think this is missing.

> Remove:
> - QModelIndex indexForItem( const KFileItem& ) const;
> - QModelIndex indexForUrl(const QUrl& url) const;
> * reasoning for the above: these are convenient functions, they don't
> belong in this class. If a user wants them then just subclass it and
> add those convenient functions.

Well, that's the point of convenience, to be convenient. Honestly I don't see 
the problem you have with these two small methods, and I'm sure that if you 
use lxr.kde.org you'll find plenty of use cases for them.

> - void expandToUrl(const QUrl& url);
> * reasoning: expanding to some folder can be done bet using
> setRootIndex (in the view)

Wrong.
setRootIndex changes the root, while expandToUrl is about expanding in a tree.
It took enough time to get that functionality right, trust me when I say it's 
not trivial. And the fact that you don't need it in a QML context (for lack of 
trees) doesn't mean the functionality can be simply removed, there are 
qtreeviews on top of KDirModel.

> - void itemChanged( const QModelIndex& index );
> * perhaps as an internal API, certainly not visible for the outside world.

Yes, the only user (KMimeTypeResolver) is already deprecated, so I agree on 
this one.

> - static QList<QUrl> simplifiedUrlList( const QList<QUrl> & urls );
> * Not the right place.

Agreed, but it's needed inside KDirModel and outside. Better suggestions for 
moving this code are welcome, but these don't make sense:

> Either re-implement the model 

How does that help with the internal call to simplifiedUrlList?

> or use KIO::ListDir (or even KDirLister)

I don't see the relation at all.

> - void requestSequenceIcon(const QModelIndex& index, int sequenceIndex);
> - void needSequenceIcon(const QModelIndex& index, int sequenceIndex);
> * Yes, but should be done internally and automatically. The user
> should only have an option to enable/disable this.

Not sure how this works exactly. Feel free to adapt 
kfile/kfilepreviewgenerator.cpp.

Overall, I'm very much against a "V2 with only half the implementation". What 
happens then? How do we ever remove V1, if V2 doesn't have what we need?
We'll stay with two "competing" implementations forever? This doesn't sound 
very appealing.
Do this incrementally - deprecate/remove what can be deprecated/removed in the 
current KDirModel, porting the calling code at the same time, which is the 
best way to ensure that something can indeed be deprecated/removed.

Creating a V2 and not porting existing code is really leaving a mess for 
others to clean up, which is why I'm strongly opposed to that approach.

> --- KDirListerV2 API ---
> For the KDirListerV2 i want to get rid of all KFileItem mentions. 

I veto this very much. It's what KDirLister is all about -- listing items.
Make KFileItem smaller in memory, make KDirLister emit them in batches rather 
than all in one go, but don't remove KFileItem altogether, it's what 
KDirLister is actually listing...

> Remove:
> -  void started( const QUrl& _url ); // You know the object and url
> your're listening to. No need to emit the url.
> -  void completed( const QUrl& _url ); // see above
> -  void canceled( const QUrl& _url ); // see above
> -  void clear( const QUrl& _url ); // see above

Not true, in the case of trees.

> -  void itemsFilteredByMime( const KFileItemList& items ); // Wrong
> place to filter

Right. This predates model/view. Feel free to deprecate/remove after checking 
the calling code (lxr.kde.org) and porting away from it within kdelibs.

> Remove:
> -  virtual void updateDirectory( const QUrl& _dir );

Agreed, seems unused from the outside.

> -  KFileItemList itemsForDir( const QUrl& dir,

Seems useful and harmless.
kfilepreviewgenerator.cpp: 702:  items << dirLister->itemsForDir(url);

> -  KFileItemList items( WhichItems which = FilteredItems ) const;

Remove the filtering, but items() is surely needed...

> -  static KFileItem cachedItemForUrl(const QUrl& url);

grep is your friend (this is used in copyjob.cpp and deletejob.cpp).

> -  bool autoErrorHandlingEnabled() const; // Adios QWidget
> -  void setAutoErrorHandlingEnabled( bool enable, QWidget *parent );
> // Adios QWidget
> -  void setMainWindow( QWidget *window ); // Adios QWidget
> -  QWidget *mainWindow(); // Adios QWidget

Agreed. (Not only because of QML, but also because nowadays apps prefer to be 
in control of error handling and show it embedded rather than popping up 
message boxes). We'll just have to make sure to actually handle errors in apps 
:-)

> -  bool dirOnlyMode() const;
> -  virtual void setDirOnlyMode( bool dirsOnly );

Well, although filtering is conceptually better done higher up ... it sounds 
like a directory chooser would be much slower if KDirLister was emitting all 
files too, just for them to be filtered afterwards. This sounds like an possibly 
useful optimisation... To be tested, I guess.

> -  bool showingDotFiles() const;
> -  virtual void setShowingDotFiles( bool _showDotFiles );

A bit of the same issue (when you have 400 dot files in your $HOME), but OK.

> -  virtual KFileItem findByUrl( const QUrl& _url ) const;
> -  virtual KFileItem findByName( const QString& name ) const;

Should not be virtual, that's for sure. About deprecating/removing, maybe, 
needs to be investigated (hard to use lxr for that, better compile KDE SC with 
this private, and see who uses it).

> Next up (in this already way too long mail) is UDSEntryV2. Right now
> if you have a massive folder the UDSEntry is quite high in the call
> costs (far from the top, but you will notice it due to the hash
> table). And it doesn't even need a hash for it to function. Thus i
> would like to implement a significantly lighter UDSEntry. One that
> uses a Vector so the memory is also aligned + very fast lookup.

Funny. It used to be a vector, I made it a hash after benchmarking.
You can find that code in kio/tests/jobtest.cpp, JobTest::newApiPerformance
It's inside #if 0, but it can be reenabled.

You need to change the iterations count for something meaningful:
    const int iterations = 30000 * 200;
Well, porting this to QBENCHMARK would be even better...

Here are the results I get with Qt4 (my Qt5 build is broken right now) :

QDEBUG : JobTest::newApiPerformance() Timing old api...
QDEBUG : JobTest::newApiPerformance() Old API: slave code: 6
QDEBUG : JobTest::newApiPerformance() Old API: app code: 8
QDEBUG : JobTest::newApiPerformance() Timing new QHash+QVariant api...
QDEBUG : JobTest::newApiPerformance() QHash+QVariant API: slave code: 12
QDEBUG : JobTest::newApiPerformance() QHash+QVariant API: app code: 7
QDEBUG : JobTest::newApiPerformance() Timing new QHash+struct api...
QDEBUG : JobTest::newApiPerformance() QHash+struct API: slave code: 8
QDEBUG : JobTest::newApiPerformance() QHash+struct API: app code: 5
QDEBUG : JobTest::newApiPerformance() Timing new QMap+struct api...
QDEBUG : JobTest::newApiPerformance() QMap+struct API: slave code: 9
QDEBUG : JobTest::newApiPerformance() QMap+struct API: app code: 5

This seems similar to what the commit log for 
dd1e5b64a69b26dc12616d27d1539b6584ebb86c says. QHash+struct wins, since app 
performance matters more than slave performance.

Feel free to look further into this and suggest a better data structure -- 
after backing it up with numbers :)

> Another thing is the "UDS_EXTRA" and "UDS_EXTRA_END". I
> seriously doubt that the functionality is even used anywhere.

Yes, this got lost with the move from the old konq views to dolphin.

> So my
> proposed structure for UDSEntryV2 would look somewhat like this:
> QVector<QString>

String to int conversion isn't fast, this is why we have ints "ready to use" 
in UDSEntry.

> I would even go as far as making a fixed length QVector. 

... wasting a lot of memory, in case the kioslave doesn't fill in everything.

> However, the current UDS stuff also has some knowledge about how a
> value should be stored (string, int, ...) so if that information is
> "really" required (which i doubt) then the following structure would
> also work:
> QVector<QVariant>

And you call this fast? :-)

See fillUDSEntryHV, I tested that idea too, back then.

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE, in particular KDE Frameworks 5