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

[Milian Wolff]

mwolff added a comment.


  In https://phabricator.kde.org/D7995#168405, @rjvbb wrote:
  
  > Milian Wolff wrote on 20171116::10:55:31 re: "https://phabricator.kde.org/D7995: KDevelop: address dirwatching inefficiency (WIP/PoC)"
  >
  > >   I **still** think the number of watched directories is completely useless information. The total time is interesting, and whether the unit tests still work that ensure the correct dirs are watched. This means: remove the project watcher class, keep using dirwatcher directly.
  >
  > We still disagree on that subject then and I can still keep the benchmark crippled if that's what you want. Did you actually read the argument why I reintroduced this output, how total time is now dependent on a number of watched directories that is no longer an external given but is now essentially an *un*controlled variable.
  >
  > Consider 2 directories with exactly the same number of files and directories. One has a substantial subtree under a folder that will be filtered out, the other not. Which one do you think will import faster, and do you think you'll understand that difference at a glance from benchmark output that doesn't includes total time but not the number of timed operations?
  
  
  The benchmark changes its performance based on the commit which hopefully improves the performance. The commit message also includes the numbers and shows the advantage of the new behavior.
  
  > Ultimately I don't care because I don't write that benchmark for myself - but removing this output will be done under protest and I'll be obliged to leave some trace of that in a comment.
  
  I won't accept it with traces of useless stuff in it.
  
  > Now, the ProjectWatcher class. This one doesn't only keep count of the number of watched directories. It also prevents directories from being added multiple times which leads to unclear behaviour inside KDirWatch and may ultimately lead to directories that aren't unwatched when necessary. So it'll have to stay.
  
  Unclear behavior? That needs to be fixed in KDirWatch upstream then.
  
  >> please document why queuing is required here?
  > 
  > Will do, but that'll just say it's following good-practice instructions from Qt devs. Qt is indeed supposed be able to detect when queued connections are required but it cannot do so 100% reliably and this is a case where I've seen it go wrong
  
  FUD. I'm a Qt dev, and I told you how it works which should be reliable for your purpose here.
  
  > (You may not remember, but I also tested making the other connections queued, which shouldn't a prior have any impact AFAIU. It did: caused lock-ups.)
  
  If you make stuff queued it does change everything - i.e. direct signal emission will never happen, even when the threads are the same.

REPOSITORY
  R32 KDevelop

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

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