KIO::ListJob::entries emits twice for folders with 100+ entries. Why?

[Mark]

On Thu, Nov 29, 2012 at 2:23 PM, David Faure <faure at kde.org> wrote:
> On Friday 23 November 2012 22:23:24 Mark wrote:
>> Here it is: http://paste.kde.org/613670/
>
> This calls gettimeofday for every entry, even when this value isn't needed.
> Can you move this to the locations where it's actually needed, for performance
> reasons?
>
> Even better would be to port this to QDateTime::currentDateTime or
> QTime::currentTime, to make this more portable (to Windows, without kdewin32).
> Or to use QTime::elapsed, which might be even simpler to use for this purpose.

Good one! I will use QTime::elapsed. The reason for moving
gettimeofday to the top is because it was being called "nearly"
always. Unless you had less then 100 entries, then it would only be
called once in the main else statement. However, this change requires
me to know the time difference every time an entry is inserted so i
moved it to the top of the function.
>
>> The changes:
>> - It's actually doing batching now regardless of the details level
>> - Batching is time based only (300ms). So if the listing takes longer
>> then 300ms then all entries thus far will be send. That continues till
>> all entries are send.
>
> It continues ... i.e. it waits for another 300ms before sending the second
> batch, right? We don't want to send them one by one, that's for sure.

In the most negative situation that "might" happen. So if you are on a
very slow network drive that for whatever reason only lists 1 entry
every 300 ms then you would indeed get 1 signal per entry. Please do
note, this is a corner case and "usually" doesn't happen. But i think
broadcasting the entries signal every 300 ms with whatever data is
there should be how it's done. (the current patch)
>
> OK so overall this change means there is no need to call totalSize() at the
> beginning of a directory listing. This should be reflected in the SlaveBase
> documentation.

Yes.
>
>> - and (while at it) simplified file_unix.cpp's FileProtocol::listDir
>> function so that it only follows one path for listing folders. It
>> still has a check for details in it, but this "looks" cleaner to me.
>> And there is no more putting it in a list and iterating over that one
>> for the details != 0 case.
>
> 1) The comment "fast path, mostly for recursive deletion", should stay under
> if (details==0).

Will do.
>
> 2) Don't add spaces inside parenthesis (...) where there weren't any before.
> Even though old kdelibs code is a bit inconsistent, the goal is to move
> towards the kdelibs coding style - like Qt: no spaces in (...).

.. i was trying to follow the current style in there. I will just do
it as Qt does it :)
>
> 3) What happened to the code that was doing a chdir back to the orig dir?

Ehh, i dropped it :p I saw no need for that to change back. The folder
is being changed to the one your listing prior to actually listing the
folder so i didn't see a need to put that back. Do you?
>
>> I tested this out on a folder with 100.000 files in it and in my case
>> i do get 2 entries signals now with no detail level provided (thus
>> quite a few details).
>> I get only one signal with all the entries in it if i pass details as
>> 0. Now it behaves as i was expecting it in the first place.
>
> Sounds good.
> Please also test:
> * the unittests in kdelibs/kio/tests  (in particular jobtest and
> kdirlistertest, but just run them all with "make test")
> * a slow remote server

Will do. I will also add the results in the reviewboard entry.
>
>> Do you guys want me to put this on reviewboard or can i just push it
>> to 4.10? My patches are never perfect the first time so i guess you
>> guys will have some comments first :p
>
> Yep, please put on reviewboard after fixing the above, it will be easier to
> review the new code than to have it intermixed with the old lines in that diff.
> I might have more comments then.

Right, will do. I hope to add the entry to reviewboard today otherwise
it will be there tomorrow. If you could still reply on the questions i
added in this reply?
>
> --
> David Faure, faure at kde.org, http://www.davidfaure.fr
> Working on KDE, in particular KDE Frameworks 5
>