<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107520/">http://git.reviewboard.kde.org/r/107520/</a>
</td>
</tr>
</table>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/107520/diff/2/?file=96862#file96862line82" style="color: black; font-weight: bold; text-decoration: underline;">kio/kio/slavebase.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class SlaveBasePrivate {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">82</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt"><span class="hl">int</span></span><span class="hl"> </span><span class="n">listEntry<span class="hl">CurrentSize</span></span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">82</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n"><span class="hl">QTime</span></span><span class="hl"> </span><span class="n"><span class="hl">m_</span>listEntry</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This variable name is confusing, I think it should be renamed to something like m_timeSinceLastBatch. Anything with "time" in the name, at least.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/107520/diff/2/?file=96863#file96863line364" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/file/file_unix.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void FileProtocol::listDir( const KUrl& url)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">364</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">errno</span> <span class="o">==</span> <span class="n">EACCES</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This assumes that QDir::setCurrent sets errno. I don't know if that's true or not, but it sounds like something that could change at any time.
I guess the kio error code distinction doesn't matter that much, so I would remove the if and just send the generic error ERR_CANNOT_ENTER_DIRECTORY.
Note: the call to finished() should be removed, it's redundant after error() and gives a warning. And in fact after error, we should stop there, with a "return" statement, that was missing in the original code.
I guess this actually never happens in practice, for opendir() to succeed and chdir() to fail...</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/107520/diff/2/?file=96863#file96863line386" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/file/file_unix.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void FileProtocol::listDir( const KUrl& url)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">368</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// Fast path (for recursive deletion, mostly)</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">381</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// Fast path (for recursive deletion, mostly)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Remove this comment, it is wrong here.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/107520/diff/2/?file=96863#file96863line387" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/file/file_unix.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void FileProtocol::listDir( const KUrl& url)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">369</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// Simply emit the name and file type, nothing else.</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">382</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// Simply emit the name and file type, nothing else.</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Move this comment down to the details == 0 code path.</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On November 29th, 2012, 11:10 p.m., Mark Gaiser wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs and David Faure.</div>
<div>By Mark Gaiser.</div>
<p style="color: grey;"><i>Updated Nov. 29, 2012, 11:10 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">listDir is batching only in a vague manner. It required to have details set to something bigger then 0 and required to know the amount of files that where going to be send. Even then it was fixed to broadcast only the first 100 entries followed by _everything_ that comes after it. For more details, look at the diff. If you did happen to do a request with details set at 0 (thus only the filename and filetype are stored) then a different code path was taken where it wasn't even possible to batch.
I changed this to do time based batching. Now it broadcasts all the entries it currently has after 300ms have been passed. In normal situations on the local file system that means that you will never see batching. You will start to see them if you hit folders with 10.000+ entries in then (for a SSD that is). For the slower HDD drives you probably see more batching.
In the worst case scenario you could find yourself in the situation that 1 entry is broadcasted every 300ms. That is only on very slow external devices or internet places.
The advantage of batching (just repeating that here since it's currently not working in any KDE app) is that you can start showing results to the user without all the results being in. This gives the user a "feeling" that it's fast even though KIO might still be working hard to get all entries in. Dolphin is one app in particular that can really make use of this. Currently it doesn't do that. IT waits till all the results are in.
While working on this, i also updated the style of the FileProtocol::listDir function and greatly simplified how it lists the directory. Now the actual listing just follows one code path instead of one for details 0 and one for the rest.
I'd like to get this in kdelibs for KDE 4.10 because it is a bugfix even though there is no bug report.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I tested this in various ways:
(SSD) 100.000 entries in a folder with details set to 0: 1 signal with all entries
(SSD) 100.000 entries in a folder without details set (all info): 2 or 3 signals each with between 20.000 - 50.000 entries
(slow ftp) ftp://ftp.mirror.aarnet.edu.au/pub/archlinux/extra/os/x86_64/ results start slowly with 20 - 50 per signal and quickly climb up.
Ran the kio test cases. Apparently i have a "failed" for kio-krununittest, a testcase that David Faure has fixed if i'm right.
Updated the documentation to not mention the total files anymore. KioSlaves can still set it, but it's just unused for the listDir now.
Last, running with this now and i haven't seen any issues so far.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kio/kio/slavebase.h <span style="color: grey">(0bdb146)</span></li>
<li>kio/kio/slavebase.cpp <span style="color: grey">(8f8ff03)</span></li>
<li>kioslave/file/file_unix.cpp <span style="color: grey">(2b47b7c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107520/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>