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

[Milian Wolff]

mwolff added a subscriber: dfaure.
mwolff added a comment.


  You didn't add the `ProjectWatcher` code. Note that your statement regarding reentrancy makes it sounds like you do not understand thread safety vs. reentrancy. If you access the same KDirWatch from multiple threads, you _have_ to protect it via mutices. But at that point, I'm actually not sure whether that is enough... we'll have to talk to @dfaure about this... Is it enough to protect the KDirWatch from the outside with a mutex? I mean, what happens when an inotify event comes in - where is that being handled? Who guarantees that this is not going to get run in parallel to code we protect by our own mutex?

INLINE COMMENTS

> iprojectcontroller.h:123
> +     */
> +    virtual bool watchAllProjectDirectories();
> +

remove, I don't see the need for this. all project dirs should always be watched

> abstractfilemanagerplugin.cpp:122
>      }
> -    delete m_watchers.take(project);
> +    QElapsedTimer timer;
> +    if (m_watchers.contains(project)) {

the whole timer code must be hidden behind ifdefs, or better yet, removed overall and instead a benchmark should measure the time for that without littering the codebase here

> abstractfilemanagerplugin.cpp:126
> +    }
> +    ProjectWatcher* watcher = m_watchers.value(project, nullptr);
> +    int nWatched = 0;

if (auto* watcher = m_watchers.take(project)) {
      delete watcher;
  }

> abstractfilemanagerplugin.cpp:142
> +    ProjectWatcher* watcher;
> +    if (ICore::self()->projectController()->watchAllProjectDirectories() && m_watchers.contains(item->project())) {
> +        watcher = m_watchers[item->project()];

remove, always watch dirs

> abstractfilemanagerplugin.cpp:288
>          }
> -        foreach ( ProjectFolderItem* parentItem, p->foldersForPath(indexedParent) ) {
> -            if ( info.isDir() ) {

why was this removed?

> abstractfilemanagerplugin.cpp:391
>      }
> -    Q_ASSERT(m_watchers.contains(folder->project()));
>      const QString path = folder->path().toLocalFile();

undo

> abstractfilemanagerplugin.cpp:411
>      const QString path = folder->path().toLocalFile();
> -    m_watchers[folder->project()]->restartDirScan(path);
> +    if (ICore::self()->projectController()->watchAllProjectDirectories()) {
> +        // restart the watcher on if @p folder corresponds to a directory

undo, remove new code - always watch all dirs

> abstractfilemanagerplugin.cpp:451
>      }
> +    ProjectWatcher* watcher = m_watchers.value(folder->project(), nullptr);
> +    if (watcher) {

auto, and assert the watcher, it must exist for a directory

> abstractfilemanagerplugin.cpp:477
>  
> -        connect(d->m_watchers[project], &KDirWatch::created,
> -                this, [&] (const QString& path_) { d->created(path_); });

why don't we listen to created anymore?

> abstractfilemanagerplugin.cpp:498
>  
> -        connect(d->m_watchers[project], &KDirWatch::created,
> -                this, [&] (const QString& path_) { d->created(path_); });
> +        // set up the signal handling; feeding the dirwatcher is handled elsewhere.
> +        connect(d->m_watchers[project], &KDirWatch::dirty,

specify _where_ it is handled

> filemanagerlistjob.cpp:42
>      qRegisterMetaType<KJob*>();
> +    setObjectName(QStringLiteral("FileManager list job for project") + item->project()->name());
>  

allocates memory for no apparent gain, remove? or just use the project name, that and the class name is enough debug info

> filemanagerlistjob.cpp:80
>  
> +void FileManagerListJob::maybeWatch(const QString& path)
> +{

remove, always watch dirs

> filemanagerlistjob.h:25
>  #include <QQueue>
> +#include <QSet>
>  

unused

> filemanagerlistjob.h:44
>  public:
> -    explicit FileManagerListJob(ProjectFolderItem* item);
> +    explicit FileManagerListJob(ProjectFolderItem* item, ProjectWatcher* watcher = nullptr);
>      ProjectFolderItem* item() const;

why is the watcher optional?

> filemanagerlistjob.h:79
> +    void maybeWatch(const QString& path);
> +    // local copy of the project's dirwatcher
> +    ProjectWatcher* m_watcher;

it's not a copy, it's a reference via pointer (you only copy the pointer). remove the comment, it doesn't add any value

> projectconfig.kcfg:26
>      </entry>
> +    <entry name="watchAllProjectDirectories" key="Monitor All Project Directories" type="Bool" default="true">
> +        <default>true</default>

remove

REPOSITORY
  R32 KDevelop

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

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