D16265: [Scheduler] Use flag to track when a runner is going idle

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
bruns marked an inline comment as done.
Closed by commit R293:b13453360685: [Scheduler] Use flag to track when a runner 
is going idle (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16265?vs=43766=44267

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

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/fileindexscheduler.h

To: bruns, #baloo, #frameworks, poboiko, ngraham, lbeltrame
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16265: [Scheduler] Use flag to track when a runner is going idle

[Luca Beltrame]

lbeltrame accepted this revision.
lbeltrame added a comment.
This revision is now accepted and ready to land.


  The changes look sane to me. Perhaps wait a couple more days until any other 
objection is raised, then if not, commit away.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, poboiko, ngraham, lbeltrame
Cc: lbeltrame, kde-frameworks-devel, ashaposhnikov, michaelh, astippich, 
spoorun, ngraham, bruns, abrahams

D16265: [Scheduler] Use flag to track when a runner is going idle

[Stefan Brüns]

bruns edited the summary of this revision.

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, poboiko, ngraham
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16265: [Scheduler] Use flag to track when a runner is going idle

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added inline comments.

INLINE COMMENTS

> poboiko wrote in fileindexscheduler.h:57
> But the very same check is already in the first line of `scheduleIndexing`, 
> why do we need it also here?

Not strictly necessary, but avoids to schedule a timer we will discard anyway

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, poboiko, ngraham
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16265: [Scheduler] Use flag to track when a runner is going idle

[Stefan Brüns]

bruns added a comment.


  So now we just need someone who accepts it 

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, poboiko, ngraham
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16265: [Scheduler] Use flag to track when a runner is going idle

[Igor Poboiko]

poboiko added a comment.


  I like it, it's better than D15959: Wait for the extraction process to finish 
before scheduling . 
  And it seems to be working, as far as I can see :)

INLINE COMMENTS

> fileindexscheduler.h:57
>  m_newFiles << file;
> -QTimer::singleShot(0, this, 
> ::scheduleIndexing);
> +if (m_indexerState == Idle || m_isGoingIdle) {
> +QTimer::singleShot(0, this, 
> ::scheduleIndexing);

But the very same check is already in the first line of `scheduleIndexing`, why 
do we need it also here?

REPOSITORY
  R293 Baloo

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

To: bruns, #baloo, #frameworks, poboiko, ngraham
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16265: [Scheduler] Use flag to track when a runner is going idle

[Stefan Brüns]

bruns updated this revision to Diff 43766.
bruns added a comment.


  rebase

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16265?vs=43765=43766

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

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/fileindexscheduler.h

To: bruns, #baloo, #frameworks, poboiko, ngraham
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams

D16265: [Scheduler] Use flag to track when a runner is going idle

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Baloo, Frameworks, poboiko, ngraham.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  Only one runner should be queued/running at a given time, but the current
  condition "m_threadPool.activeThreadCount == 0" is to strong, as the
  done() signal is fired from the runner thread while it is still alive and
  may be processed before the thread has exited.
  
  Set a flag when the runner is finished, and use this to guard from
  scheduling/enqueuing multiple runners. It is fine to enqueue another
  runner while the old is cleaning up, the new one will become active
  afterwards.
  
  Alternative approach to D15959 .

TEST PLAN
  start `balooctl monitor~
  touch a file -> file is indexed, scheduler goes idle afterwards
  touch multiple files, new files -> indexing, idle

REPOSITORY
  R293 Baloo

BRANCH
  mimetype_handling

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

AFFECTED FILES
  src/file/fileindexscheduler.cpp
  src/file/fileindexscheduler.h

To: bruns, #baloo, #frameworks, poboiko, ngraham
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams