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

[Milian Wolff]

mwolff added a comment.


  the benchmark is required for testing this properly. Also it allows us to verify the thread safety using helgrind or tsan

INLINE COMMENTS

> abstractfilemanagerplugin.cpp:130
> +#endif
> +    delete m_watchers.value(project, nullptr);
> +    m_watchers.remove(project);

delete + take like before

> abstractfilemanagerplugin.cpp:289
> -        foreach ( ProjectFolderItem* parentItem, p->foldersForPath(indexedParent) ) {
> -            if ( info.isDir() ) {
> -                ProjectFolderItem* folder = q->createFolderItem( p, path, parentItem );

ok so this code was already dead before your patch, it seems (cf. loc 273-283 above) - so this can be removed separately from your patch to minimize the diff

> abstractfilemanagerplugin.cpp:297
> -            } else {
> -                ProjectFileItem* file = q->createFileItem( p, path, parentItem );
> -                if (file) {

this one needs to stay in master, it added new files to the folder

> abstractfilemanagerplugin.cpp:394
>      const QString path = folder->path().toLocalFile();
> -    m_watchers[folder->project()]->stopDirScan(path);
> +    ProjectWatcher* watcher = m_watchers.value(folder->project(), nullptr);
> +    if (watcher && watcher->contains(path)) {

undo the change hunk completely

> abstractfilemanagerplugin.cpp:409
> +    if (!m_watchers[folder->project()]->restartDirScan(path)) {
> +        // path wasn't being watched yet - should never happen
> +        // (but can we be 100% certain of that?)

it worked before, is this really required now? if not, you can make that a separate patch if you think it solves an issue. to me it looks like a blind change that has nothing to do with the actual thing you are trying to fix here

> projectwatcher.cpp:33
> +public:
> +    static QMutex m_barrier;
> +};

a static mutex? why? make it one mutex per dirwatch

also call it m_mutex, not barrier

> projectwatcher.h:34
> +
> +    void addDir(const QString& path, WatchModes watchModes=WatchDirOnly);
> +    void removeDir(const QString& path);

whitespaces around operators

> projectwatcher.h:38
> +private:
> +    class Private;
> +    Private* d;

don't pimple, this class is not going to be exported

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, mwolff
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/93a8d581/attachment-0001.html>