Sync: Avoid 3 passes over SyncDarta DB when loading the directory from the disk

Sync: Avoid 3 passes over SyncDarta DB when loading the directory from the disk

According to profiling the first two read passes over SyncData DB
are from DirectoryBackingStore::DropDeletedEntries. This function
executes two SQL queries that drop entries pending deletion.
Since it is uncommon to have entries pending deletion it is far
more efficient to remove DirectoryBackingStore::DropDeletedEntries
and instead check for the entries matching this criteria inside
DirectoryBackingStore::LoadEntries, skip those entries from being
loaded and put them straight into Directory's metahandles_to_purge
collection which sets them up for deletion during a subsequent database
save.

See the bug for the preliminary performance results.

BUG=464073
Committed: https://crrev.com/a3ed68bbb72ec9fdf16d866f41cb0951d006088b
Cr-Commit-Position: refs/heads/master@{#320861}

[Nicolas Zea, 2015-03-16 19:33:36]

zea@chromium.org changed reviewers:
+ zea@chromium.org

[Nicolas Zea, 2015-03-16 19:33:36]

This looks good. Two questions: where are we now doing the actual purging of
those entries? Do we have any tests to verify that we eventually purge these
entries?

https://codereview.chromium.org/1008103002/diff/1/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/1008103002/diff/1/sync/syncable/directory.h#n...
sync/syncable/directory.h:604: // Used by VacuumAfterSaveChanges.
nit: may as well put a newline above (and below the method declaration), as I
don't think these three methods need to be grouped together.

[stanisc, 2015-03-16 20:43:50]

On 2015/03/16 19:33:36, Nicolas Zea wrote:
> This looks good. Two questions: where are we now doing the actual purging of
> those entries? Do we have any tests to verify that we eventually purge these
> entries?
> 
> https://codereview.chromium.org/1008103002/diff/1/sync/syncable/directory.h
> File sync/syncable/directory.h (right):
> 
>
https://codereview.chromium.org/1008103002/diff/1/sync/syncable/directory.h#n...
> sync/syncable/directory.h:604: // Used by VacuumAfterSaveChanges.
> nit: may as well put a newline above (and below the method declaration), as I
> don't think these three methods need to be grouped together.

Thanks! Just wanted to validate the general approach first.
The actual purging is done on subsequent Save.
There is no test yet but I plan to add one shortly.

[stanisc, 2015-03-17 00:54:48]

Added a unit test that verifies purging of deleted entries.
Added performance results to the bug.

[Nicolas Zea, 2015-03-17 01:05:17]

LGTM

https://codereview.chromium.org/1008103002/diff/20001/sync/syncable/directory...
File sync/syncable/directory_unittest.cc (right):

https://codereview.chromium.org/1008103002/diff/20001/sync/syncable/directory...
sync/syncable/directory_unittest.cc:559: const int CLIENT_COUNT = 2;
nit: const variables should be of the form kCamelCase

https://codereview.chromium.org/1008103002/diff/20001/sync/syncable/directory...
sync/syncable/directory_unittest.cc:613: // Expect items 0 and 5 to be purged
nit: explain why?

[stanisc, 2015-03-17 04:36:53]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2015-03-17 04:36:53]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1008103002/#ps40001
(title: "Addressed CR feedback.")

[commit-bot: I haz the power, 2015-03-17 04:37:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1008103002/40001

[commit-bot: I haz the power, 2015-03-17 04:40:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-17 04:41:04]

Patchset 3 (id:??) landed as
https://crrev.com/a3ed68bbb72ec9fdf16d866f41cb0951d006088b
Cr-Commit-Position: refs/heads/master@{#320861}