<table><tr><td style="">rjvbb added inline comments.
</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><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-33707" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.cpp:123</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">If you find this overly useful (I don't personally, but OK), please commit this part right away with the following code:</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);">#ifdef TIME_IMPORT_JOB
QElapsedTimer timer;
timer.start();
#endif

delete m_watchers.take(project);

#ifdef TIME_IMPORT_JOB
qCDebug(FILEMANAGER) << "Deleting dir watcher took" << timer.elapsed() / 1000.0 << "seconds for project" << project->name();
#endif</pre></div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not exactly like that: the timer should only be started when the project is still being watched. Without that conditional the debug statement can print multiple additional times with a 0s timing.</p>

<p style="padding: 0; margin: 8px;">I'll push to 5.2 and hold off updating this rev. until the commit has been merged by someone.</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-33708" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.h:106</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">remove this, it's imo not useful to know from the benchmark number and its an approximation anyways</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The benchmark timings do depend on this number, so they really only means something when printed with the number of watched items. At least when the number is correct, so if I remove it here I'll have to find another way to obtain the info in the follow-up patch.</p>

<p style="padding: 0; margin: 8px;">IMHO this is all the more true if the improved algorithm refrains from watching folders that are filtered out because that means you cannot even do a simple <tt style="background: #ebebeb; font-size: 13px;">find foo -type d | wc -l</tt> to determine the number of directories under <tt style="background: #ebebeb; font-size: 13px;">foo</tt>.</p>

<p style="padding: 0; margin: 8px;">Actually, I could already introduce the ProjectWatcher class (it no longer needs a mutex), with or without the bit that applies the project filter. That should make the number of watched items available in the benchmark via <tt style="background: #ebebeb; font-size: 13px;">dynamic_cast<ProjectWatcher*>(m_manager->projectWatcher(m_project))</tt>. Not watching likely filter suspects like .git and build should reduce import times a bit already so I like the idea, let me know if that's acceptable?</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-33709" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.h:118</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">remove. if necessary connect to <tt style="background: #ebebeb; font-size: 13px;">IProjectController::projectClosing</tt> internally, but don't let others call this directly</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Can you please explain how that would work without creating a ProjectController instance in the benchmark?<br />
I do get a non-null result from <tt style="background: #ebebeb; font-size: 13px;">projectController()</tt> in the benchmark app but when I get a crash when I do <tt style="background: #ebebeb; font-size: 13px;">emit projectController()->projectClosing(m_project)</tt>. I don't understand why, the only explanation that makes sense to me is that the method returns an uninitialised IProjectController pointer.<br />
The signal could also be sent from the Project instance itself rather than from the ProjectController.</p>

<p style="padding: 0; margin: 8px;">Is there no other way to call this hidden code without patching code that's otherwise completely unrelated to dirwatching issues?</p>

<p style="padding: 0; margin: 8px;">PLEASE don't just look at the patch, *try* it locally, look at the numbers you get with the most relevant KDirWatch methods and decide whether or not the time spent deleting dirwatchers is really irrelevant to you.</p>

<p style="padding: 0; margin: 8px;">I honestly don't really understand why AFMP couldn't have a (protected) method that basically does the opposite of what the <tt style="background: #ebebeb; font-size: 13px;">import</tt> method does.</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-33714" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerpluginimportbenchmark.cpp:75</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">don't output a project number, us the project name/path only</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">OK here, but not in the other output (knowing the completion order is useful IMHO).</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-33715" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerpluginimportbenchmark.cpp:111</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">imo it's more useful to know the number of files and dirs rather than the number of watched items (that should be an implementation detail, cf. comment above)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What comment are you referring to?</p>

<p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">fileSet().size()</tt> gives the number of files and dirs. In the follow-up patch only dirs will be watched, so the 2 values being printed will give provide the number of files and the number of dirs in a way that I consider most useful to assess the impact of dirwatcher initialisation. (The benchmark will print the actual time of that step in the current code.)</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>