Differential D7995 Diff 19960 kdevplatform/project/projectwatcher.cpp

Changeset View

Standalone View

kdevplatform/project/projectwatcher.cpp

This file was added.

1		/***************************************************************************
2		* This file is part of KDevelop *
3		* Copyright 2017 René Bertin <rjvbertin@gmail.com> *
4		* *
5		* This program is free software; you can redistribute it and/or modify *
6		* it under the terms of the GNU Library General Public License as *
7		* published by the Free Software Foundation; either version 2 of the *
8		* License, or (at your option) any later version. *
9		* *
10		* This program is distributed in the hope that it will be useful, *
11		* but WITHOUT ANY WARRANTY; without even the implied warranty of *
12		* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
13		* GNU General Public License for more details. *
14		* *
15		* You should have received a copy of the GNU Library General Public *
16		* License along with this program; if not, write to the *
17		* Free Software Foundation, Inc., *
18		* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. *
19		***************************************************************************/
20
21		#include "projectwatcher.h"
22
23		#include <QMutex>
24		#include <QMutexLocker>
25
26		#include <KDirWatch>
27
28		using namespace KDevelop;
29
30		class KDevelop::ProjectWatcher::Private
31		{
32		public:
33		static QMutex m_barrier;
	mwolffUnsubmitted Not Done a static mutex? why? make it one mutex per dirwatch also call it m_mutex, not barrier mwolff: a static mutex? why? make it one mutex per dirwatch also call it m_mutex, not barrier
	rjvbbAuthorUnsubmitted Not Done No, that doesn't work properly across all platforms. Remember how I said KDirWatch uses a single QFileSystemWatcher instance to handle all KDirWatch instances using the QFSW method? Because of that this code should never be executed in parallel, and I wanted to use a name that explains a bit better what the thing does than just `m_mutex`. KDW could use a static mutex internally to protect its QFSW instance but we'd still be needing a mutex here so I'd propose to stick with a single, static external mutex for now. `KDW::addDir()` of a singe directory is cheap and fast compared to the other things going on, and chances that this method is being called concurrently are slim anyway so the potential performance penalty should be negligible. As mentioned elsewhere: I've only seen the effects of not using a static mutex on Mac. Without mutex or with non-static mutex instances I get a significant increase in import times (variable but up to about 4x) and sometimes deadlocks occur (deep in the FSEvents backend to QFileSystemWatcher). This happens only when loading multiple projects that have a large number of almost empty directories, IOW, that call KDW::addDir() in rapid succession from multiple threads. The only possible effect I've seen on Linux of not using a static mutex is an almost anecdotal double free deep in the KDW implementation. I haven't even been able to reproduce it but I'd never seen that happen before in all the years that I've been using KDevelop so I think that was some kind of race condition. rjvbb: No, that doesn't work properly across all platforms. Remember how I said KDirWatch uses a…
	mwolffUnsubmitted Not Done Can you please clarify this: Are you saying different KDirWatcher instances share the same QFileSystemWatcher? If so, that needs to be changed upstream, but I'd be OK with working around that for now. mwolff: Can you please clarify this: Are you saying different KDirWatcher instances share the same…
	rjvbbAuthorUnsubmitted Not Done Indeed, that's my understanding from looking over kdirwatch.cpp (which is a mess to read for outsiders). @dfaure : do you know if there was a reason to use a single QFSW instance and if that reason is still valid? I could easily imagine (for instance) that it'd be a good idea to use a single QFSW instance per volume (disk, partition) and that in practice that boils down to using a single QFSW instance. I'll have to check if the Mac FSEvents backend uses a single native instance. I should also try to understand why its own internal mutex didn't prevent the concurrency issues I mentioned above (emphasis on "try" :-/) rjvbb: Indeed, that's my understanding from looking over kdirwatch.cpp (which is a mess to read for…
	aaronpuchertUnsubmitted Not Done Calling a mutex `m_barrier` is a bit misleading: mutex comes from mutual exclusion. While that includes a memory barrier (preventing reordering around `lock` and `unlock`), it's more than that. Memory barriers don't guarantee that only one thread can access a resource at a given time. On top of that, there is an actual synchronization mechanism called barrier, which is something entirely different. aaronpuchert: Calling a mutex `m_barrier` is a bit misleading: mutex comes from mutual exclusion.
34		};
35
36		QMutex KDevelop::ProjectWatcher::Private::m_barrier;
37
38		KDevelop::ProjectWatcher::ProjectWatcher(QObject* parent)
39		: KDirWatch(parent)
	mwolffUnsubmitted Done i.e. move this comment into the header, it applies to the mutex in general, not only to this part of the usage mwolff: i.e. move this comment into the header, it applies to the mutex in general, not only to this…
40		, d(new Private)
41		{}
42
43		KDevelop::ProjectWatcher::~ProjectWatcher()
44		{
45		delete d;
46		}
47
48		void KDevelop::ProjectWatcher::addDir(const QString& path, WatchModes watchModes)
49		{
50		// The QFileSystemWatcher backend doesn't like to be called
51		// too often/concurrently; prevent concurrent calls (can happen
52		// in trees with lots of few-element directories).
53		// (On Mac, this improves speeds, on Linux I've seen the occasional
54		// memory allocation issue without this barrier.)
55		QMutexLocker lower(&Private::m_barrier);
56		if (!contains(path)) {
57		KDirWatch::addDir(path, watchModes);
58		}
59		}
60
61		void KDevelop::ProjectWatcher::removeDir(const QString& path)
62		{
63		QMutexLocker lower(&Private::m_barrier);
64		if (contains(path)) {
65		KDirWatch::removeDir(path);
66		}
67		}