Summary: | Dirwatcher inefficiency when using QFileSystemWatcher | ||
---|---|---|---|
Product: | [Developer tools] kdevplatform | Reporter: | RJVB <rjvbertin> |
Component: | filemanager | Assignee: | kdevelop-bugs-null |
Status: | VERIFIED UNMAINTAINED | ||
Severity: | normal | CC: | mail, simonandric5 |
Priority: | NOR | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Platform: | Compiled Sources | ||
OS: | All | ||
See Also: | https://bugs.kde.org/show_bug.cgi?id=387238 | ||
Latest Commit: | Version Fixed In: | ||
Attachments: |
profiling overview on Mac, dirwatcher creation
profiling overview on Mac, dirwatcher destruction on-demand set-up of dir-only dirwatching on-demand set-up of dir-only dirwatching |
Description
RJVB
2017-09-20 12:46:28 UTC
Thanks for creating this report so we can track this properly. Some notes: a) - kdev should only watch directories, CMakeLists.txt files and open files for changes - the CMakeLists.txt used to be required, now that we use the cmake server it may not be required anymore, we'll have to check that again. - so if you say all files are watched then this needs to be fixed - it shouldn't be the case b) - if using QFSWatch shows hotspots in memory allocation and string comparison, then those should be fixed in QFileSystemWatcher upstream. Writing a separate benchmark (standalone from kdevelop) for that purpose would help a lot. I imagine KDirWatch would contain a benchmark, if not, then one should be added that resembles the workload you are seeing. Similarly, such a benchmark would be helpful upstream in Qt itself So I think a) is the actual bug you have uncovered and that needs to be fixed. b) can then also be done on top of that, to improve the situation even further If CMakeLists.txt files are to be monitored that should be an additional action taken by the CMake project manager, not by a generic file manager. And that was once necessary it should not be removed until support for older cmake versions is officially and completely removed. (Which in turn shouldn't happen until cmake server mode works as reliably and efficiently as the json-based mode, IMHO).
The venom is in this call:
d->m_watchers[project]->addDir(project->path().toLocalFile(), KDirWatch::WatchSubDirs | KDirWatch:: WatchFiles );
Removing the KDirWatch::WatchFiles should already be an improvement, but I'd hope that doing the addDir() only for folders that are actually being loaded might at the same time avoid putting a monitor on excluded folders. Assuming that's OK of course.
> b)
Maybe. I've queried the Qt ML for feedback on this, and for the existence of a dedicated benchmark but I'm going to go with the null hypothesis that the QFSW API isn't new and has had ample opportunity for fine-tuning by Qt experts. We're just using it in a very intensive way here.
I think that the GCC source tree contains almost 8000 files in I don't know how many directories. It doesn't strike me as odd that memory allocation and string comparisons show up as hotspots when creating or deleting that many QFSW entries involve that kind of operation. I'm a bit surprised that "only" 8000 or so allocations and string comparisons can be that expensive in terms of processing time but not incredulous. It's really not something I want to dig into at the moment.
Created attachment 107915 [details]
profiling overview on Mac, dirwatcher creation
Created attachment 107916 [details]
profiling overview on Mac, dirwatcher destruction
a) if my memory serves me right (I've written this a long time ago), I think the WatchFiles there may have been required to get notified about file deletion events. Anyhow, I agree that this needs to be improved somehow. Maybe this isn't needed anymore, maybe it was never required in the first place. Writing better tests for this would be the first way to fix this. b) You said: "I'm going to go with the null hypothesis that the QFSW API isn't new and has had ample opportunity for fine-tuning by Qt experts. We're just using it in a very intensive way here." From my long time experience working with Qt, this shouldn't be your null hypothesis. I've found and fixed quite a few glaring performance issues in Qt over the years, and most simply arose when you pushed a bit more load at the public API in ways that apparently noone did so far. Maybe we do that here due to the misconfiguration in a). Nevertheless ruling out performance optimizations upstream is a very bad idea at this stage. Just looking at the profile reports, QStringList pops up - a very bad idea if that's getting queries in O(N) all the time. Using a QSet or sorted vector could already dramatically improve the runtime performance. I see that KDirWatch was first written in '98. If you wrote this code that long ago too, then it's probably past due that someone looks at it... And the same can evidently be said about "well-preserved" APIs in Qt. Funny, really, in evolutionary biology that term is mostly seen as something positive :) I didn't mean I write KDirWatch, only the usage of that API in KDevelop is mostly my code nowadays. Still, I think I wrote that 5-7 years ago or so, while it was still only part of the generic manager. Either way, KDirWatch *is* that old. Most code in it was last touched during a Jenkins commit in 2013 (transition to KF5?). Some work has been done on it recently but still it too checks each and every entry being added against a list of already added entries (and its contains() implementation looks like it might not be optimised at all). Optimising this code is closer to home than QFSW even if the main cost is clearly with QFSW. Anyway I've been doing some tinkering. Curiously it seems that removing the KDirWatch::WatchFiles flag from the addDir() call *increases* the time spent in that method when the QFSW method is used (and by no small amount). OTOH, using KDirWatch::WatchDirOnly (in eventuallyLoadDirectory()) and the dirty instead of the created signal gives a dirwatcher that seems to work as it should (for the toplevel folder and any folders I reload manually). It even works on Mac. I found an unsent email in which I intended to ask if deferring feeding the dirwatcher to the FileManagerListJob class would be an acceptable approach. That's the class that apparently does the actual project loading, and I'd thus expect it to be the logical place to add individual project directories, on a backgrond thread and with minimal event loop blocking. Created attachment 108017 [details]
on-demand set-up of dir-only dirwatching
This is a PoC implementation of my idea of deferring the dirwatcher feeding to the worker that does the actual project directory traversal (the FileManagerListJob class), for now coupled with the possibility to disable dirwatching for my own convenience.
Initial tests show that
- big project import is a *lot* faster
- dirwatching actually works on Mac.
I've used a QSet to avoid repetitive calls to KDirWatch::contains(). I haven't timed it, but judging from the latter's implementation the former should be considerably faster.
I hope I haven't missed a place or two where entries have to be removed from the dirwatcher.
Let me know if this is mature enough to phab it?
Created attachment 108020 [details]
on-demand set-up of dir-only dirwatching
If you want to have it reviewed, put it up on phabricator - attaching it here isn't helping anyone and just wastes our time. Regarding QSet (without reading the code): Why not push that into KDirWatch to get the efficient lookup everywhere, not just in KDevelop? As I said, please upstream everything possible. > Why not push that into KDirWatch to get the efficient lookup everywhere, not just in KDevelop?
And then what, let everyone who doesn't have the latest frameworks version stick with the non-optimised version?
Yes. That is no sufficient reason to go with the technically inferior version in this case. Yes. FWIW, none of my former employers (national scientific research institutions) would buy software from vendors with that kind of attitude. I'll propose the approach upstream and probably the lock too. But I'm not going to allow myself to get side-tracked in a lengthy and costly review and optimisation process of a utility class that may well only benefit of it in the very specific case of working with big projects in KDevelop. Well, we do distribute the AppImage for a reason. I think that the import timings obtained with the recent abstractfilemanagerpluginbenchmark show that there is a problem with the current dirwatcher implementation when it's not using the INotify backend, in addition to the fact that watching all project entries is inappropriate in that case. Limitations on other platforms suggest that only project directories and their subdirectories should be watched and D7995 has shown that one can make a feature-complete dirwatching implementation based on that principle that also removes all risk of a UI lockup. Anyone picking this up should however be aware of #387238 which will go from a hypothetical to a real potential issue. |