606353 - History sync: use mozIAsyncHistory::updatePlaces

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

need to actually add them first...

Comment 1

[Mike Connor [:mconnor]]

From discussing with sdwilsh, this is what it'll take for Sync to avoid triggering bug 606062, which really just looks like a specific case of bug 563538, which is why it's blocking final releases on desktop/mobile.  Still figuring out resources, but one way or another this needs to happen.

Comment 3

[Philipp von Weitershausen [:philikon]]

Reflect the current state of this bug: so far only mozIAsyncHistory::updatePlaces has been implemented, so we can only do this for history sync for now. Filed bug 626279 for the bookmark stuff (which still requires bug 519514 to be implemented in the first place).

Comment 4

[Philipp von Weitershausen [:philikon]]

I have outlined a proposal for changing the store/engine API to allow batching in bug 622762 comment 11. With this in place, the history engine will apply incoming records in batches by calling mozIAsyncHistory::updatePlaces with multiple records at a time.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: Preq (v1): Utils.checkGUID

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: Part 0: Improve and harden history sync tests

The new places API is much more strict so a bunch of tests have to be hardened to produce valid GUIDs and valid visits (WHY-OH-WHY does test_corrupt_keys use actual engines???).

Also added tests for the kinds of records we won't (can't) be accepting in the future. Some of these fail with just this patch applied as the actual checking isn't happening yet.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 507601 [details] [diff] [review]
Preq (v1): Utils.checkGUID

Wouldn't it be more efficient to just grab https://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/toolkit/components/places/tests/head_common.js#l603?  Might want to ask cdleary or someone else on the JS team.

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1: Use mozIAsyncHistory::updatePlaces when available

This refactors the history engine to use mozIAsyncHistory::updatePlaces when available. Makes the tests in part 0 pass.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 507601 [details] [diff] [review]
Preq (v1): Utils.checkGUID

Any reason not to do:

var re = /^[-abcdefghijklmnopqrstuvwxyz0123456789_]{12}$/i;
function checkGUID(guid) {
  return !!guid && re.test(guid);
}

In informal testing on my machine, this is about 30x faster than trolling over the array calling a JS function.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to comment #9)
> Any reason not to do:
> 
> var re = /^[-abcdefghijklmnopqrstuvwxyz0123456789_]{12}$/i;
> function checkGUID(guid) {
>   return !!guid && re.test(guid);
> }

My (irrational?) hatred of regular expressions.

> In informal testing on my machine, this is about 30x faster than trolling over
> the array calling a JS function.

Pretty convincing. I'll fix.

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: Preq (v2): Utils.checkGUID

For queen and country.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 507617 [details] [diff] [review]
Preq (v2): Utils.checkGUID

Make it so!

Comment 13

[Philipp von Weitershausen [:philikon]]

Try build: http://tbpl.mozilla.org/?tree=MozillaTry&rev=ed45880ddada

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 507603 [details] [diff] [review]
Part 0: Improve and harden history sync tests

1. Can you pull out 

  Engines.get("history")._store

in lines 176 and 228 onwards, rather than wrapping the lines?


2. In test_history_store.js: please align those dump() calls. (Also, dumpn rather than + "\n"?)


3. These checks:

  do_check_eq(failed[0], no_date_visit_guid);

look like they would fail if we switched representations (internally or externally) and lost the ordering. Is there any reason to assume an ordering in the result here? If not, can you put a .sort() in there before running the checks?

Otherwise, looks good, and tests pass with just this patch in my queue.

Comment 15

[Richard Newman [:rnewman]]

(In reply to comment #14)

> Otherwise, looks good, and tests pass with just this patch in my queue.

Scratch that. Did I mention how much I hate mq?

Test fails because applyIncomingBatch doesn't exist, of course. Validating with the other patch now.

Comment 16

[Philipp von Weitershausen [:philikon]]

(In reply to comment #14)
> 1. Can you pull out 
> 
>   Engines.get("history")._store
> 
> in lines 176 and 228 onwards, rather than wrapping the lines?

Will do.

> 2. In test_history_store.js: please align those dump() calls. (Also, dumpn
> rather than + "\n"?)

Bah, these aren't supposed to be in there. I just added them for debugging. Will remove.

> 3. These checks:
> 
>   do_check_eq(failed[0], no_date_visit_guid);
> 
> look like they would fail if we switched representations (internally or
> externally) and lost the ordering. Is there any reason to assume an ordering in
> the result here? If not, can you put a .sort() in there before running the
> checks?

Good catch. No reason to assume ordering. In other places I have used .sort(), so will do that.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 507605 [details] [diff] [review]
Part 1: Use mozIAsyncHistory::updatePlaces when available

Patch fails to apply on top of tests:

applying bug-606353-1.patch
patching file services/sync/modules/constants.js
Hunk #1 FAILED at 84
1 out of 1 hunks FAILED -- saving rejects to file services/sync/modules/constants.js.rej
patching file services/sync/modules/engines/history.js
Hunk #2 FAILED at 71
1 out of 5 hunks FAILED -- saving rejects to file services/sync/modules/engines/history.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug-606353-1.patch

I believe I'm missing some constants from another patch in your queue. Easy enough to get it to apply by hand, but consequently take this review with a pinch of salt.


One error in test_history_store, right after "Let's modify the record and have the store update the database.":

[Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [mozIAsyncHistory.updatePlaces]"

presumably because I'm not running with the necessary Places changes.

Comments:

450 }, this).filter(function (placeInfo) {
451 	return placeInfo;
452 });

This took me a minute to figure out, and I'm a functional programmer! Please consider defining:

  function identity(x) x;

and using

  .filter(identity);

instead. The functional style is much clearer when the language exposes all the right vocabulary...


Typo:

495  this._log.trace("Ignorning record " + record.id +

and again in 529.


Otherwise, looks good...

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to comment #17)
> Patch fails to apply on top of tests:
...
> I believe I'm missing some constants from another patch in your queue. Easy
> enough to get it to apply by hand, but consequently take this review with a
> pinch of salt.

Erm, the test patch doesn't even touch history.js or constants.js. This is what my patch queue looks like:

 0 A bug622762: Bug 622762 - Add batch API for incoming records, deal with exceptions in SyncEngine's recordHandler
 1 A bug606353-tests: Bug 606353 - Improve and harden history sync tests
 2 A bug606353-util: Bug 606353 - Add a Utils.checkGUID() helper
 3 A bug606353: Bug 606353 - History sync: use mozIAsyncHistory::updatePlaces

It applied cleanly on all of my three checkouts (fx-sync, places, mozilla-central).

> One error in test_history_store, right after "Let's modify the record and have
> the store update the database.":
> 
> [Exception... "Component returned failure code: 0x80004001
> (NS_ERROR_NOT_IMPLEMENTED) [mozIAsyncHistory.updatePlaces]"
> 
> presumably because I'm not running with the necessary Places changes.

Update your mozilla-central build?

> Comments:
> 
> 450 }, this).filter(function (placeInfo) {
> 451     return placeInfo;
> 452 });
> 
> This took me a minute to figure out, and I'm a functional programmer! Please
> consider defining:
> 
>   function identity(x) x;
> 
> and using
> 
>   .filter(identity);
> 
> instead. The functional style is much clearer when the language exposes all the
> right vocabulary...

Can do.

Comment 19

[Richard Newman [:rnewman]]

(In reply to comment #18)

> my patch queue looks like:

Yup, was just missing a dep.
 
> Update your mozilla-central build?

Done. 97 test passes. Thumbs up!

(Marking as ASSIGNED, btw.)

Comment 20

[Philipp von Weitershausen [:philikon]]

Attachment: Part 0 (v1.1): Improve and harden history sync tests

Address rnewman's review comments from Comment 14.

Comment 21

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1.1): Use mozIAsyncHistory::updatePlaces when available

Address rnewman's review comments:
* Introduce function identity for the filter
* Fix typos

Comment 22

[Philipp von Weitershausen [:philikon]]

Crossweave tests with latest tinderbox build (which has mozIAsyncHistory::updatePlaces) pass. Just needs bug 622762 to land now, otherwise good to go!

Comment 23

[Philipp von Weitershausen [:philikon]]

New try build: http://tbpl.mozilla.org/?tree=MozillaTry&rev=1eddde320380

Comment 24

[Philipp von Weitershausen [:philikon]]

Part 0: https://hg.mozilla.org/services/fx-sync/rev/833e18225829
Preq: https://hg.mozilla.org/services/fx-sync/rev/2afdc6b14ad5
Part 1: https://hg.mozilla.org/services/fx-sync/rev/5dc7db5e0192

Comment 25

[Tony Chung [:tchung]]

Verified on fennec nightlies with Mozilla/5.0 (Android; Linux armv71; rv:2.0b11pre) Gecko/20110204 Firefox/4.0b11pre Fennec/4.0b5pre.   Tested on a Nexus One, Nexus S.  

Most of the testing involved:
- syncing history only, large records  (2.5 MB)
- disabling sync logging
- strip out form data history records
- sync on fennec, and pan over to content pages to resume functional tests (panning, zooming, open a new tab, loading web content, typing into url and search boxes)

Result: user performance during sync was noticeably faster than when i did the same scenario on a nightly build prior to this fix.

Since this is a large patch, awaiting others to comment before marking verified.  Also, looking for feedback on desktop testing.