D8059: KDevelop: abstractfilemanagerimport benchmark

[René J.V. Bertin]

rjvbb added inline comments.

INLINE COMMENTS

> mwolff wrote in abstractfilemanagerplugin.cpp:123
> If you find this overly useful (I don't personally, but OK), please commit this part right away with the following code:
> 
>   #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

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.

I'll push to 5.2 and hold off updating this rev. until the commit has been merged by someone.

> mwolff wrote in abstractfilemanagerplugin.h:106
> remove this, it's imo not useful to know from the benchmark number and its an approximation anyways

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.

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 `find foo -type d | wc -l` to determine the number of directories under `foo`.

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 `dynamic_cast<ProjectWatcher*>(m_manager->projectWatcher(m_project))`. 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?

> mwolff wrote in abstractfilemanagerplugin.h:118
> remove. if necessary connect to `IProjectController::projectClosing` internally, but don't let others call this directly

Can you please explain how that would work without creating a ProjectController instance in the benchmark?
I do get a non-null result from `projectController()` in the benchmark app but when I get a crash when I do `emit projectController()->projectClosing(m_project)`. I don't understand why, the only explanation that makes sense to me is that the method returns an uninitialised IProjectController pointer.
The signal could also be sent from the Project instance itself rather than from the ProjectController.

Is there no other way to call this hidden code without patching code that's otherwise completely unrelated to dirwatching issues?

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.

I honestly don't really understand why AFMP couldn't have a (protected) method that basically does the opposite of what the `import` method does.

> mwolff wrote in abstractfilemanagerpluginimportbenchmark.cpp:75
> don't output a project number, us the project name/path only

OK here, but not in the other output (knowing the completion order is useful IMHO).

> mwolff wrote in abstractfilemanagerpluginimportbenchmark.cpp:111
> 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)

What comment are you referring to?

`fileSet().size()` 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.)

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D8059

To: rjvbb, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20171001/ea102982/attachment.html>