<table><tr><td style="">mwolff requested changes to this revision.<br />mwolff added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D8059" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>the commit message should include some output numbers and document how to run this test with different backends. I.e. something like the following should be the desired commit message _and_ also shows the desired output from the benchmark itself:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">On my (linux) machine, running the benchmark on the kdevelop master repo, I measure the following:
KDIRWATCH_METHOD=inotify ./kdevplatform/project/tests/abstractfilemanagerpluginimportbenchmark ~/projects/kf5/src/extragear/kdevelop/kdevelop/
KDirWatch backend: Inotify
Benchmark set-up took 0.152 seconds
Starting import of project "/home/milian/projects/kf5/src/extragear/kdevelop/kdevelop"
creating dirwatcher took 0.021 seconds
imported project 0 with 5611 elements (XXX files & YYY folders) took 0.062 seconds
Done in 0.236 seconds total (excluding dirwatcher destruction)
Closing project
Done in XYZ seconds total
KDIRWATCH_METHOD=QFSWatch ./kdevplatform/project/tests/abstractfilemanagerpluginimportbenchmark ~/projects/kf5/src/extragear/kdevelop/kdevelop/
KDirWatch backend: QFSWatch
Benchmark set-up took 0.181 seconds
Starting import of project "/home/milian/projects/kf5/src/extragear/kdevelop/kdevelop"
creating dirwatcher took 0.14 seconds
imported project 0 with 5611 elements (XXX files & YYY folders) took 0.066 seconds
Done in 0.388 seconds total (excluding dirwatcher destruction)
Closing project
Done in XYZ seconds total</pre></div>
<p>Note that the number of calls to <tt style="background: #ebebeb; font-size: 13px;">addDir</tt> is completely useless output (it's an implementation detail). What is interesting, is the total time only.</p>
<p>Note also how for me, the QSFWatch dirwatcher creation only takes 0.14s for the kdevelop project, which is still pretty fast. Using FAM takes ages though. Please do add the numbers for your mac machine, too.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8059#inline-35497" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">projectwatcher.cpp:39</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">m_filter</span><span style="color: #aa2211">-></span><span class="n">isValid</span><span class="p">(</span><span class="n">Path</span><span class="p">(</span><span class="n">path</span><span class="p">),</span> <span style="color: #304a96">true</span><span class="p">,</span> <span class="n">m_project</span><span class="p">)</span> <span style="color: #aa2211">&&</span> <span style="color: #aa2211">!</span><span class="n">contains</span><span class="p">(</span><span class="n">path</span><span class="p">))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">KDirWatch</span><span style="color: #aa2211">::</span><span class="n">addDir</span><span class="p">(</span><span class="n">path</span><span class="p">,</span> <span class="n">watchModes</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">this filter check here is conceptually different functionality compared to before. I'm not saying it's a bad change, but it makes this patch do more than just adding a benchmark. Please split your patches up to make it easier to review _and_ to keep the history sane.</p>
<p style="padding: 0; margin: 8px;">Splitting up this patch that way will also allow us to assess the impact of this change.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8059#inline-35495" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerpluginimportbenchmark.cpp:44</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #aa4000">class</span> <span style="color: #00702a">AFMPBenchmark</span> <span style="color: #aa2211">:</span> <span style="color: #aa4000">public</span> <span class="n">QObject</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Name the class after the file: <tt style="background: #ebebeb; font-size: 13px;">AbstractFileManagerPluginImportBenchmark</tt></p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8059#inline-35496" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerpluginimportbenchmark.cpp:58</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">m_projectNumber</span> <span style="color: #aa2211">=</span> <span class="n">s_numBenchmarksRunning</span><span style="color: #aa2211">++</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">qInfo</span><span class="p">()</span> <span style="color: #aa2211"><<</span> <span style="color: #766510">"Starting import of project"</span> <span style="color: #aa2211"><<</span> <span class="n">m_project</span><span style="color: #aa2211">-></span><span class="n">path</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">m_timer</span><span class="p">.</span><span class="n">start</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">here and below: don't use qInfo, it's usually not enabled. Instead, use <tt style="background: #ebebeb; font-size: 13px;">qDebug</tt> or better yet use a <tt style="background: #ebebeb; font-size: 13px;">QTextStream m_out</tt> member and initialize it with <tt style="background: #ebebeb; font-size: 13px;">m_out(stdout)</tt>.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D8059" rel="noreferrer">https://phabricator.kde.org/D8059</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, mwolff<br /><strong>Cc: </strong>mwolff, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger<br /></div>