KDirModelV2, KDirListerV2 and UDSEntryV2 suggestions

Sat Dec 29 14:21:26 UTC 2012

On Sat, Dec 29, 2012 at 12:44 PM, David Faure <faure at kde.org> wrote:
> 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).

That certainly is an interesting alternative approach!
Just a little side note, is it possible to get rid of KFileItem and
use QFileInfo? Though i'm guessing it's not possible due to Qt not
being as powerful as KIO in the file stuff.
>
>> 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.

This kinda depends on it. Why would you cache? Because something is
slow. If the reason for it being slow is gone then you don't need a
cache at all. That's my aim here. Making it fast enough for the cache
to be removed. Perhaps the UDSEntry should have all the values of
KFileItem where KFileItem would just access those values. That would
make the UDSEntry a little more heavier but would make KFileItem
nearly free.
>
>> --- 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.

True. When i wrote that i was thinking about all the other changes in
KDirLister(V2) thus i thought it would heavily influence the KDirModel
implementation as well. Thus a V2 of that one. But you're right, it
doesn't need a V2, just a big cleanup :)
>
>> - 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.

Well if that's the case then i would go for not implementing it at
all. Users can still make a custom KDirModel and implement it if they
like.
>
>> 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.

Missed that one. Oke, should not be removed/deprecated. Though i do
wonder how QFileSystemModel is doing this..
>
>> - 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.

There is none :)
Now that i think of it again, it might be best for the KDirLister to
play this role. It has all the data anyway.

>
>> - 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.

True. Perhaps a better name would be "KDirModelAlternative" ;) Anyway,
i agree with you. It doesn't need a V2, it only needs a cleanup, a
update and some deprecated lines.
>
>> --- 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...

You even VETO this.. wow, i never had a veto before :)
Anyway, i do understand why you veto this. I personally want to keep
as little information in this class as possible yet as complete as
possible. All data that can be "calculated" based on data in this
class would be ok for me. That's why i wanted to get KFileItem out
since it can be fully constructed from the UDSEntry. Another reason is
that you don't even need all the KFileItem instances. You never see
them all anyway (in big folders).

So i think KDirLister needs two lists.
1 list with all the UDSEntry entries (AKA UDSEntryList)
1 list with the constructed KFileItem lists + lazy loading them when needed.

>
>> 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.
Right, with this i also wonder how QFileSystemModel is doing this. It
doesn't have those functions but does "function" the same way.. Those
functions at least don't have to be public, right?
>
>> -  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...
true
>
>> -  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.

I think you're mixing things up a bit now. The intention is to set
flags (like in QDir). If you set the flag to list directory only then
you obviously only get the signals for directories. Same for files,
dot files and whatever else is possible.
>
>> -  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).

My guess it that it's used quite a bit. It's convenience, but
something that should be done by the user. Just get the items from the
listener and search in that list. Not something that should be placed
in the listener itself.
>
>> 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.

Not everything will do a string to int conversion.
For example:
QVector<QString> file;
file.value(UDS_NAME);

That's a fast lookup with no conversion at all.
However, file.value(UDS_SIZE); will have the downside that it has to
be converted to a int..

All in all the UDSEntry container is a simple int -> value lookup, it
doesn't need a hash, but it does need "some" logic to determine it's
value type. This needs some further consideration. Tricky issue!

>
>> 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.

ehh true. Then i guess it should only be done when details = 0 thus
making a vector of just 2 elements.

>
>> 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? :-)
Nope, needs some more consideration.
>
> See fillUDSEntryHV, I tested that idea too, back then.
Will do, thanks for the pointer.
>
> --
> David Faure, faure at kde.org, http://www.davidfaure.fr
> Working on KDE, in particular KDE Frameworks 5
>
> _______________________________________________
> Kde-frameworks-devel mailing list
> Kde-frameworks-devel at kde.org
> https://mail.kde.org/mailman/listinfo/kde-frameworks-devel