D8059: KDevelop: abstractfilemanagerimport benchmark

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> abstractfilemanagerplugin.cpp:123
>      }
> +#ifdef TIME_IMPORT_JOB
> +    QElapsedTimer timer;

If you find this overly useful (I don't personally, but OK), please commit this part right away with the following code:

  #ifdef TIME_IMPORT_JOB
  QElapsedTimer timer;
  timer.start();
  #endif
  
  delete m_watchers.take(project);
  
  #ifdef TIME_IMPORT_JOB
  qCDebug(FILEMANAGER) << "Deleting dir watcher took" << timer.elapsed() / 1000.0 << "seconds for project" << project->name();
  #endif

> abstractfilemanagerplugin.h:106
>  
> +    /**
> +     * @return the number of watched directories for the given @p project.

remove this, it's imo not useful to know from the benchmark number and its an approximation anyways

> abstractfilemanagerplugin.h:118
> +     */
> +    void projectClosing( IProject *project );
> +

remove. if necessary connect to `IProjectController::projectClosing` internally, but don't let others call this directly

> abstractfilemanagerpluginimportbenchmark.cpp:41
> +
> +class TestFileManagerPlugin : public AbstractFileManagerPlugin
> +{

remove, once the two new functions are gone

> abstractfilemanagerpluginimportbenchmark.cpp:54
> +
> +class AFMPBenchmark : public QObject {
> +    Q_OBJECT

{ on its own line

> abstractfilemanagerpluginimportbenchmark.cpp:60
> +    {
> +        if (QFileInfo(path).isDir()) {
> +            m_manager = manager;

should be handled outside the creation of this object (i.e. don't create an instance when this invariant isn't upheld)

> abstractfilemanagerpluginimportbenchmark.cpp:71
> +    {
> +        if (!m_project) {
> +            return;

remove once above is fixed

> abstractfilemanagerpluginimportbenchmark.cpp:75
> +        m_projectNumber = s_count++;
> +        qInfo() << "Starting import of project #" << m_projectNumber << "at" << m_project->path();
> +        m_timer.start();

don't output a project number, us the project name/path only

> abstractfilemanagerpluginimportbenchmark.cpp:100
> +
> +    static int s_count;
> +

`s_numBenchmarksRunning`, but I'd actually remove this as a static here and instead push it to a stack variable in main below

> abstractfilemanagerpluginimportbenchmark.cpp:111
> +        qInfo() << "imported project" << m_projectNumber
> +            << "with" << m_project->fileSet().size()
> +            << "files (watched:" << m_manager->watchedItems(m_project) << "):"

imo it's more useful to know the number of files and dirs rather than the number of watched items (that should be an implementation detail, cf. comment above)

> abstractfilemanagerpluginimportbenchmark.cpp:115
> +
> +        s_count -= 1;
> +        if (s_count <= 0) {

move down, see "MOVE_TO_HERE" marker below

> abstractfilemanagerpluginimportbenchmark.cpp:134
> +
> +    AutoTestShell::init();
> +    auto core = TestCore::initialize(Core::NoUi);

you could try to init with `AutoTestShell::init({"no plugins"})` to ensure no other plugin gets loaded

> abstractfilemanagerpluginimportbenchmark.cpp:147
> +        QObject::connect(benchmark, &AFMPBenchmark::finished,
> +                     &app, [&benchmarks] {
> +                        for (auto benchmark : benchmarks) {

indent align the `&app` with the `benchmark` above, increment indent level below (such that `});` is again on the same level)

> abstractfilemanagerpluginimportbenchmark.cpp:151
> +                        }
> +                        QCoreApplication::instance()->quit();
> +                     });

MOVE_TO_HERE

> abstractfilemanagerpluginimportbenchmark.cpp:158
> +
> +    if (benchmarks.first()->s_count > 0) {
> +        return app.exec();

if (benchmarks.isEmpty()) {
      qWarning() << "no projects to import";
      return 1;
  }
  return app.exec();

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/20171001/ea6e2169/attachment-0001.html>