Overhaul the file index scheduler.
Needs RevisionPublic

Authored by smithjd on May 30 2018, 3:48 PM.

Details

Reviewers
bruns
mgallien
Summary

Allow all operations except the content indexer to work when suspended. Allow manually resuming file content indexing while on battery. Allow some operations to run in parallel.

BUG: 378597

Diff Detail

Repository
R293 Baloo
Branch
master-scheduler (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7366
Build 7384: arc lint + arc unit
smithjd created this revision.May 30 2018, 3:48 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptMay 30 2018, 3:48 PM
Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript
smithjd requested review of this revision.May 30 2018, 3:48 PM
smithjd retitled this revision from Overhaul the file index scheduler. Allow all operations except the content indexer to work when suspended. Allow manually resuming file content indexing while on battery. balooctl stop now stops file indexing. to Overhaul the file index scheduler..May 30 2018, 3:49 PM
smithjd edited the summary of this revision. (Show Details)
smithjd updated this revision to Diff 35199.May 30 2018, 5:06 PM
  • Minor simplification + fix: call checkUnindexedFiles when folder watches are installed instead of scheduleIndexing which can't check for changes in indexable files.

Sounds good to me. I will try to do a proper review as soon as I can. Sorry for the delay.

bruns added a comment.Jun 6 2018, 11:00 PM

If I understand your summary correctly, your patch changes different things. While these are clearly related, can you try to separate the changes?

smithjd updated this revision to Diff 35730.Jun 7 2018, 12:27 AM

Split-up scheduler overhaul patchset.

  • The power state signal should only be emitted when the power state changes.
  • Re-write the the file index scheduler.
  • Update the balooctl tool with the changed suspend/resume behaviour.
  • Make use of the scheduler halt method.
  • Check for unindexed files when folder watches are installed.
bruns added a comment.Jun 7 2018, 7:36 PM

@smithjd You have probaly become another victim of phabricator/arc. Your commits have been squashed by arc ...
If you wan't your commits to stay separated, you have to do a git checkout of the first commit, do a arc diff HEAD^1, checkout the next commit, arc diff HEAD^1, and so on.

@smithjd You have probaly become another victim of phabricator/arc. Your commits have been squashed by arc ...
If you wan't your commits to stay separated, you have to do a git checkout of the first commit, do a arc diff HEAD^1, checkout the next commit, arc diff HEAD^1, and so on.

Commits are squashed on arc land anyway, but the branch can be manually pushed keeping the separate patches. If you mean separated into different reviews, I don't think it's unclear why there are small changes to code related to the scheduler, or why it might be advantageous to separate these into different reviews.

Is there any practical reason why a simple arc land wouldn't suffice here?

bruns added a comment.Jun 7 2018, 8:30 PM

@smithjd You have probaly become another victim of phabricator/arc. Your commits have been squashed by arc ...
If you wan't your commits to stay separated, you have to do a git checkout of the first commit, do a arc diff HEAD^1, checkout the next commit, arc diff HEAD^1, and so on.

Commits are squashed on arc land anyway, but the branch can be manually pushed keeping the separate patches. If you mean separated into different reviews, I don't think it's unclear why there are small changes to code related to the scheduler, or why it might be advantageous to separate these into different reviews.

Is there any practical reason why a simple arc land wouldn't suffice here?

Because pushing independent stuff in a single commit just sucks for the reviewer, and it sucks if you want to understand the git history later.

You are changing 5 different things in your patches. I can not see which changed source line relates to which change.

According to the arc documentation, a --merge commit should land the commits separately.

bruns added a comment.Jun 7 2018, 8:45 PM

According to the arc documentation, a --merge commit should land the commits separately.

Still, I can not see the individual commits.

According to the arc documentation, a --merge commit should land the commits separately.

Still, I can not see the individual commits.

That appears to be a Phabricator limitation.

ngraham added a subscriber: ngraham.Jun 7 2018, 9:06 PM

For future reference, the preferred method to implement a multi-commit patch chain is with multiple Phabricator revisions. See https://community.kde.org/Infrastructure/Phabricator#If_the_patches_are_all_for_the_same_project

smithjd updated this revision to Diff 35810.Jun 7 2018, 10:57 PM
  • Re-write the the file index scheduler.
  • Update the balooctl tool with the changed suspend/resume behaviour.
smithjd edited the summary of this revision. (Show Details)Jun 7 2018, 11:18 PM
smithjd edited the summary of this revision. (Show Details)Jun 7 2018, 11:37 PM
bruns added a comment.Jun 19 2018, 1:21 AM

This still contains two unrelated changes. Can you please submit each one individually?

smithjd updated this revision to Diff 36417.Jun 21 2018, 1:05 AM
  • Re-write the the file index scheduler.
  • Update the balooctl tool with the changed suspend/resume behaviour.
smithjd updated this revision to Diff 36701.Jun 26 2018, 5:19 PM

re-word commits.

smithjd updated this revision to Diff 40343.Aug 24 2018, 3:20 AM
  • Re-write the file index scheduler. Combine content indexer suspend logic.
  • Update the balooctl tool with the changed suspend/resume behaviour.
  • Re-order and use IndexerState to prioritize the indexer thread pool.
  • Newline and tab fixes.
  • Prevent a scheduler race condition.
  • Separate runnableStateChanged into two functors.
  • Allow the first run indexer to complete before running any other runnables. Prevent the new file and unindexed file runnables from running at the same time.
bruns requested changes to this revision.Aug 24 2018, 1:30 PM

Split this into independent changes.
Add add comprehensive description to the summary - a bug reference is insufficient.

src/file/fileindexscheduler.cpp
102

You sneak in a unmentioned and significant change - you allow to run several indexer tasks to run in parallel!

124

Whats that supposed to do (I have an idea, but thats ugly ...)

src/file/fileindexscheduler.h
95

Make this a QFlags ...

This revision now requires changes to proceed.Aug 24 2018, 1:30 PM
smithjd edited the summary of this revision. (Show Details)Aug 24 2018, 6:32 PM
bruns added a comment.Aug 24 2018, 6:43 PM

... and thats the problem - this change does 3 different things. Make each one a separate revision.

smithjd added inline comments.Aug 24 2018, 7:57 PM
src/file/fileindexscheduler.cpp
124

This starts a QRunnable at a negative priority (in this case the negative integer value for NewFiles) and then calls runnableStarted(NewFiles) with the enum of the started runnable.

src/file/fileindexscheduler.h
95

There is no cast from int to enum anywhere here. There is a cast fron enum to int, to a negative integer used to set the QRunnable thread priority.

bruns added inline comments.Aug 24 2018, 8:59 PM
src/file/fileindexscheduler.cpp
124

Yeah, I suspected that - fugly as hell!

If you need different priorities, make it explicit.

src/file/fileindexscheduler.h
95

Sigh ...
Make each state a bit in the flags. No bits set -> Idle, one or multiple bits set -> Suspended, or runners active.

smithjd updated this revision to Diff 40393.Aug 25 2018, 2:57 AM
  • Revert "Re-order and use IndexerState to prioritize the indexer thread pool."

Avoid setting a priority on the runnables.

smithjd updated this revision to Diff 43355.Oct 10 2018, 9:05 PM
smithjd marked an inline comment as done.
  • Re-write the file index scheduler. Combine content indexer suspend logic.
  • Update the balooctl tool with the changed suspend/resume behaviour.
  • Re-order and use IndexerState to prioritize the indexer thread pool.
  • Newline and tab fixes.
  • Prevent a scheduler race condition.
  • Separate runnableStateChanged into two functors.
  • Allow the first run indexer to complete before running any other runnables. Prevent the new file and unindexed file runnables from running at the same time.
  • Revert "Re-order and use IndexerState to prioritize the indexer thread pool."
  • Simplify locking the scheduler.
  • Adapt index cleaner to scheduler.
smithjd updated this revision to Diff 43623.Oct 14 2018, 8:38 PM
  • Re-write the file index scheduler. Combine content indexer suspend logic.
  • Update the balooctl tool with the changed suspend/resume behaviour.
  • Re-order and use IndexerState to prioritize the indexer thread pool.
  • Newline and tab fixes.
  • Prevent a scheduler race condition.
  • Separate runnableStateChanged into two functors.
  • Allow the first run indexer to complete before running any other runnables. Prevent the new file and unindexed file runnables from running at the same time.
  • Revert "Re-order and use IndexerState to prioritize the indexer thread pool."
  • Simplify locking the scheduler.
  • Adapt index cleaner to scheduler.

Exporting the storage object is not part of this patchset.

smithjd updated this revision to Diff 49977.Jan 21 2019, 12:41 AM
  • Re-write the file index scheduler. Combine content indexer suspend logic.
  • Update the balooctl tool with the changed suspend/resume behaviour.
  • Re-order and use IndexerState to prioritize the indexer thread pool.
  • Newline and tab fixes.
  • Prevent a scheduler race condition.
  • Separate runnableStateChanged into two functors.
  • Allow the first run indexer to complete before running any other runnables. Prevent the new file and unindexed file runnables from running at the same time.
  • Revert "Re-order and use IndexerState to prioritize the indexer thread pool."
  • Simplify locking the scheduler.
  • Adapt index cleaner to scheduler.
  • Rebase
bruns requested changes to this revision.Jan 21 2019, 12:53 AM

STOP IT!
You are reverting recent, required changes.

This revision now requires changes to proceed.Jan 21 2019, 12:53 AM

"Locking" the scheduler is simpler here than identifying that it "has gone idle". "Locking" only happens here for a small number of runnables (two) that can't run concurrently.

bruns added a comment.Jan 21 2019, 5:05 PM

"Locking" the scheduler is simpler here than identifying that it "has gone idle". "Locking" only happens here for a small number of runnables (two) that can't run concurrently.

If you don't understand the code, do not change it.

This is not about avoiding to be scheduled, but missing the schedule signal.

You are mixing a ton of different things here, clean it up!