D7995: KDevelop: address dirwatching inefficiency (WIP/PoC)

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> abstractfilemanagerplugin.cpp:126
> +    QElapsedTimer timer;
> +    if (m_watchers.contains(project)) {
> +        timer.start();

remove the conditionals, always output the time?

> abstractfilemanagerplugin.cpp:412
>      const QString path = folder->path().toLocalFile();
> +    Q_ASSERT(m_watchers.contains(folder->project()));
>      m_watchers[folder->project()]->stopDirScan(path);

move up, like before (just use `git checkout -p HEAD^` and undo this hunk)

> abstractfilemanagerplugin.cpp:503
>      if ( project->path().isLocalFile() ) {
> -        d->m_watchers[project] = new KDirWatch( project );
> +        d->m_watchers[project] = new ProjectWatcher(project);
>  

while at it, can you please clean this up to not do multiple lookups:

  auto watcher = new ProjectWatcher(project);
  connect(watcher ,...);
  connect(watcher ,...);
  d->m_watchers[project] = watcher;

> abstractfilemanagerplugin.h:106
>       */
>      KDirWatch* projectWatcher( IProject* project ) const;
>  

actually, isn't this now unsafe API? If the dir watcher needs to be protected by a mutex, we must not hand it out as a raw pointer, no?

> abstractfilemanagerplugin.h:111
> +     */
> +    int watchedItems( IProject* project ) const;
> +

this should be moved into KDirWatch, then we can use the getter above

> abstractfilemanagerplugin.h:114
> +    /**
> +     * unregisters the given @p project, deleting dirwatchers,
> +     * stopping associated jobs, etc. Intended for testing only.

so it's like closing the project, no? why do

> filemanagerlistjob.cpp:108
>                      entry.insert(KIO::UDSEntry::UDS_FILE_TYPE, QT_STAT_DIR);
> +                    m_watcher->addDir(info.absoluteFilePath());
>                  }

aren't you adding the same dir twice now? i.e. once here, and once above when the dir gets listed itself?

> projectwatcher.cpp:38
> +{
> +    // The QFileSystemWatcher backend doesn't like to be called
> +    // too often/concurrently; prevent concurrent calls (can happen

i.e. move this comment into the header, it applies to the mutex in general, not only to this part of the usage

> projectwatcher.h:37
> +
> +    int size() {
> +        return m_watchedCount;

de-inline, constify

> projectwatcher.h:42
> +private:
> +    static QMutex m_mutex;
> +    int m_watchedCount;

add a big fat comment explaining why this must be static, change `m_` prefix to `s_`

> abstractfilemanagerpluginimportbenchmark.cpp:41
> +
> +class TestFileManagerPlugin : public AbstractFileManagerPlugin {
> +    Q_OBJECT

{ on its own line

> abstractfilemanagerpluginimportbenchmark.cpp:48
> +
> +    int watchedItems(IProject* project)
> +    {

are you only changing visibility here? if so, use `using` instead, or better yet - add the `AFMPBenchmark` as friend class to the `AFMP`

> abstractfilemanagerpluginimportbenchmark.cpp:76
> +    {
> +        if (m_project) {
> +            auto root = m_manager->import(m_project);

de-inline most of this by returning early

  if (!m_project) {
      return;
  }

> abstractfilemanagerpluginimportbenchmark.cpp:138
> +
> +    QMap<KDirWatch::Method,QString> kdwMethod = {
> +        {KDirWatch::FAM, QStringLiteral("FAM")}

use a plain vector or std::array or c-array? Or are the enumerators non-contiguous? if the latter, use qhash

> abstractfilemanagerpluginimportbenchmark.cpp:158
> +                     });
> +        if (qEnvironmentVariableIsSet("BENCHMARK_ORIGINAL_DIRWATCHER")) {
> +            // benchmark the creation and deletion of the original dirwatcher:

I think you misunderstood me. I don't want to keep the benchmark of the old implementation around. What I want is a simple benchmark (separate patch!) that can be run on the current implementation. Then when one applies your optimization patch, it should show a performance improvement of the benchmark.

> abstractfilemanagerpluginimportbenchmark.cpp:172
> +    }
> +    foreach (auto benchmark, benchmarks) {
> +        benchmark->start();

range-based for

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, mwolff
Cc: aaronpuchert, arrowdodger, kfunk, dfaure, mwolff, brauch, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170928/7c51c299/attachment-0001.html>