Commit 65dfa84c authored by Lina Cambridge's avatar Lina Cambridge
Browse files

Bug 1573305 - Remove the check for unmerged synced bookmark changes. r=markh 

When a local or remote item changed, we'd potentially scan three tables
(with an expensive `LEFT JOIN`!) to check if anything changed...then
scan the same tables again to build the local and remote trees. This
check was originally meant to avoid unnecessary merges. However, the
bottleneck isn't merging now; it's reading from the database.

Since the merger has been rewritten in Rust, is synchronous, doesn't
keep a transaction open for the entire merge (see the
`total_sync_changes` check), and only emits ops for items that actually
changed, it's more efficient to build and merge optimistically, and
bail before applying if nothing changed.

This commit also moves `validateLocalRoots` into Rust.

Differential Revision: https://phabricator.services.mozilla.com/D41690

--HG--
extra : moz-landing-system : lando
parent 13a72955

Cargo.lock

+3 −3
Original line number Diff line number Diff line
@@ -385,7 +385,7 @@ version = "0.1.0" 
dependencies = [

 "atomic_refcell 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",

 "cstr 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)",

 "dogear 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)",

 "dogear 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)",

 "libc 0.2.60 (registry+https://github.com/rust-lang/crates.io-index)",

 "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",

 "moz_task 0.1.0",
@@ -956,7 +956,7 @@ dependencies = [ 


[[package]]

name = "dogear"

version = "0.3.0"

version = "0.3.1"

source = "registry+https://github.com/rust-lang/crates.io-index"

dependencies = [

 "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -3989,7 +3989,7 @@ dependencies = [ 
"checksum devd-rs 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "0d009f166c0d9e9f9909dc751630b3a6411ab7f85a153d32d01deb364ffe52a7"

"checksum digest 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "05f47366984d3ad862010e22c7ce81a7dbcaebbdfb37241a620f8b6596ee135c"

"checksum dirs 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)" = "88972de891f6118092b643d85a0b28e0678e0f948d7f879aa32f2d5aafe97d2a"

"checksum dogear 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "99e2ed5f8036ceb83d951f769651ff0e2be5ddb0c62da87d50d27c22086db01c"

"checksum dogear 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "251a15c9a597d70eb53cbb0c5473d8d8c6241aef615c092030ebab27fb5b26ef"

"checksum dtoa 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)" = "09c3753c3db574d215cba4ea76018483895d7bff25a31b49ba45db21c48e50ab"

"checksum dtoa-short 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "068d4026697c1a18f0b0bb8cfcad1b0c151b90d8edb9bf4c235ad68128920d1d"

"checksum dwrote 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)" = "0bd1369e02db5e9b842a9b67bce8a2fcc043beafb2ae8a799dd482d46ea1ff0d"

services/sync/tests/unit/test_bookmark_repair_responder.js

+15 −11
Original line number Diff line number Diff line
@@ -346,9 +346,6 @@ add_task(async function test_folder_descendants() { 
  );



  _("Insert missing bookmarks locally to request later");

  // Note that the fact we insert the bookmarks via PlacesSyncUtils.bookmarks.insert

  // means that we are pretending Sync itself wrote them, hence they aren't

  // considered "changed" locally so never get uploaded.

  let childBmk = await PlacesSyncUtils.bookmarks.insert({

    kind: "bookmark",

    recordId: Utils.makeGUID(),
@@ -371,15 +368,22 @@ add_task(async function test_folder_descendants() { 
    url: "https://mozilla.org",

  });



  _("Sync again; server contents shouldn't change");

  _("Sync again");

  await Service.sync();

  {

    // The buffered engine will upload the missing records, since it does a full

    // tree merge. The old engine won't, since it relies on the Sync status

    // flag, and we used `PSU.b.i` to pretend that Sync added the bookmarks.

    let collection = getServerBookmarks(server);

    collection.remove(childBmk.recordId);

    collection.remove(grandChildBmk.recordId);

    collection.remove(grandChildSiblingBmk.recordId);

    deepEqual(

    getServerBookmarks(server)

      .keys()

      .sort(),

      collection.keys().sort(),

      initialRecordIds,

      "Second sync should not upload missing bookmarks"

    );

  }



  // This assumes the parent record on the server is correct, and the server

  // is just missing the children. This isn't a correct assumption if the

third_party/rust/dogear/.cargo-checksum.json

+1 −1
Original line number Diff line number Diff line 
{"files":{"CODE_OF_CONDUCT.md":"e85149c44f478f164f7d5f55f6e66c9b5ae236d4a11107d5e2a93fe71dd874b9","Cargo.toml":"8a1067e5d4af68c738884e76256033d638b938c40789eefb0c078a79f5238890","LICENSE":"c71d239df91726fc519c6eb72d318ec65820627232b2f796219e87dcf35d0ab4","README.md":"303ea5ec53d4e86f2c321056e8158e31aa061353a99e52de3d76859d40919efc","src/driver.rs":"913de08a578b38902f4eb0d78147289897af5d3b1ecdad818c4ea6f83da6c714","src/error.rs":"d4ef0cba5c7fc54959ed62da166f10435548d705e0a817eed449fb001fe4e21d","src/guid.rs":"c82af64fba3ad87948a9b599241e48753d17587e8c642f610949163be3d499bf","src/lib.rs":"0606e69b235650bf404ae0b03a1e85c2063bb4b7147fa4d5e8ff2c128a757453","src/merge.rs":"1fcdc00dcf47743e72372ba85b00e5f48d0fc9b8586f9b3bd5c0b0604628d8b3","src/store.rs":"e5ce4c358b05b2483198b37c90d2f6c740c8e349c70abcee3ba914cb80f66aae","src/tests.rs":"51bc77eb989974f7594dfa0cdf7741f8e26f2956eebc34d1f71cbfd137512940","src/tree.rs":"fcdc9a5ae0e3f1b0d62d01bf1024db2a0aa8660e6839ada607a6a20e0fe2ad83"},"package":"99e2ed5f8036ceb83d951f769651ff0e2be5ddb0c62da87d50d27c22086db01c"}
 
 No newline at end of file
 
{"files":{"CODE_OF_CONDUCT.md":"e85149c44f478f164f7d5f55f6e66c9b5ae236d4a11107d5e2a93fe71dd874b9","Cargo.toml":"cfc0416b0a1c11ade3b5c11ec579c8e26ed3fc3b5871dd99d4fbcc12bd2614d0","LICENSE":"c71d239df91726fc519c6eb72d318ec65820627232b2f796219e87dcf35d0ab4","README.md":"303ea5ec53d4e86f2c321056e8158e31aa061353a99e52de3d76859d40919efc","src/driver.rs":"913de08a578b38902f4eb0d78147289897af5d3b1ecdad818c4ea6f83da6c714","src/error.rs":"d4ef0cba5c7fc54959ed62da166f10435548d705e0a817eed449fb001fe4e21d","src/guid.rs":"c82af64fba3ad87948a9b599241e48753d17587e8c642f610949163be3d499bf","src/lib.rs":"0606e69b235650bf404ae0b03a1e85c2063bb4b7147fa4d5e8ff2c128a757453","src/merge.rs":"e74727171831585d304eee3a3c7ba24fec52a5e7c6999ed4e647fefa14b8be99","src/store.rs":"629410e44485c0473617911be0e06dc5ce504c7427782be02f05a2f4096b5351","src/tests.rs":"51bc77eb989974f7594dfa0cdf7741f8e26f2956eebc34d1f71cbfd137512940","src/tree.rs":"fcdc9a5ae0e3f1b0d62d01bf1024db2a0aa8660e6839ada607a6a20e0fe2ad83"},"package":"251a15c9a597d70eb53cbb0c5473d8d8c6241aef615c092030ebab27fb5b26ef"}
 
 No newline at end of file

third_party/rust/dogear/Cargo.toml

+1 −1
Original line number Diff line number Diff line
@@ -13,7 +13,7 @@ 
[package]

edition = "2018"

name = "dogear"

version = "0.3.0"

version = "0.3.1"

authors = ["Lina Cambridge <lina@mozilla.com>"]

exclude = ["/.travis/**", ".travis.yml", "/docs/**", "book.toml"]

description = "A library for merging bookmark trees."

third_party/rust/dogear/src/merge.rs

+14 −0
Original line number Diff line number Diff line
@@ -1857,6 +1857,20 @@ pub struct CompletionOps<'t> { 
    pub upload: Vec<Upload<'t>>,

}



impl<'t> CompletionOps<'t> {

    /// Returns `true` if there are no completion ops to apply.

    #[inline]

    pub fn is_empty(&self) -> bool {

        self.change_guids.is_empty()

            && self.apply_remote_items.is_empty()

            && self.apply_new_local_structure.is_empty()

            && self.flag_for_upload.is_empty()

            && self.skip_upload.is_empty()

            && self.flag_as_merged.is_empty()

            && self.upload.is_empty()

    }

}



/// A completion op to change the local GUID to the merged GUID. This is used

/// to dedupe new local items to remote ones, as well as to fix up invalid

/// GUIDs.