D8059: KDevelop: abstractfilemanagerimport benchmark

[Milian Wolff]

mwolff added a comment.


  In https://phabricator.kde.org/D8059#157312, @rjvbb wrote:
  
  > >   Note that the number of calls to `addDir` is completely useless output (it's an implementation detail). What is interesting, is the total time only.
  >
  > I agree that this is true for the current implementation. However, it's the only thing we can measure at the moment without burdening the benchmark or the importer itself with code to count the number of directories in a project. That number of calls should be seen in the light of before and after comparison. I'm fine with meto output this only in a benchmark mod included with the actual dirwatcher modification (in which case it can be printed as "number of dirs watched" since addDir() will be adding a single dir only).
  
  
  Again - no! The important number is the time spent. Anything else is not interesting at all. The idea to output the dirs and files of a project (which can be done quite easily using the ProjectModel API) would give some secondary information. But it's not crucial at all.
  
  The number of calls to addDir is totally uninteresting. Even more so as before it was/is recursive internally while you are about to change it. So what value is there when the benchmark shows "1 call to addDir" now, and then eventually "1000 calls to addDir"?!
  
  Again: We only care about the time it takes to import the project.
  
  >>   Note also how for me, the QSFWatch dirwatcher creation only takes 0.14s for the kdevelop project, which is still pretty fast. 
  > 
  > Maybe you have a fast SSD and had been doing enough things in that project directory that the entire tree structure was cached? I systematically get much longer durations, though they do stabilise on lower numbers than what I can get when the tree structure isn't cached at all.
  
  On Linux or on Mac? One way or another, this clearly shows (again, I keep repeating myself) that we need data from a proper profiler. Please do run it through `perf` as I have requested multiple times. As a first step, at least run the benchmark through `perf stat` as that would show us already whether you are suffering from IO or CPU load.
  
  > FWIW, I'm currently seeing 13115 elements under my working copy of the master branch (and still 12896 when I move the .git directory out of the way). Of those, 9202 are entries from the in-source build directory, which contains *only* the results of running cmake, *not* of building.
  
  So the issue for you is maybe that you are tracking the build folder as part of the project? What happens when you move that build dir to a sibling folder?
  
  > Maybe you could also try with the gcc 7.2.0 source tree?
  
  
  
  >> Splitting up this patch that way will also allow us to assess the impact of this change.
  > 
  > So I should defer this review to get that basic change in, for instance the introduction of a ProjectWatcher class?
  
  No, you should first submit this benchmark without changing any other code. As I said above the ProjectWatcher class is not required for this benchmark.
  
  > Is that going to be taking as long as this review? I'm not against doing it, but it shouldn't make management of my local patches completely unworkable either so please do provide any feedback that could speed up getting that change in (preferably still into the 5.2 branch). Feel free to email them to keep them out of this ticket.
  
  Reviewing takes as long as it takes. You cannot magically make it faster.
  
  >> here and below: don't use qInfo, it's usually not enabled. Instead, use `qDebug` or better yet use a `QTextStream m_out` member and initialize it with `m_out(stdout)`.
  > 
  > I thought that too, but got corrected on another ticket (I think by dfaure): qInfo *is* enabled by default, or was it qCInfo? I'll use QTextStream and just hope it'll print everything as qInfo c.s. do.
  
  Do use `QTextStream` please.

REPOSITORY
  R32 KDevelop

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

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