Closed Bug 606353 Opened 9 years ago Closed 9 years ago

History sync: use mozIAsyncHistory::updatePlaces

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
fennec 2.0b5+ ---

People

(Reporter: mconnor, Assigned: philikon)

References

Details

(Whiteboard: [hardblocker][has patch][has review][fx4-fixed-bugday])

Attachments

(3 files, 3 obsolete files)

need to actually add them first...
blocking2.0: --- → final+
Target Milestone: --- → 2.0
Blocks: 606062
Depends on: 606966
No longer depends on: 606350
tracking-fennec: --- → 2.0+
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.
Assignee: nobody → mconnor
tracking-fennec: 2.0+ → 2.0b3+
Duplicate of this bug: 606062
tracking-fennec: 2.0b3+ → 2.0b4+
Whiteboard: [hardblocker]
Assignee: mconnor → philipp
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).
No longer depends on: placesAsyncBookmarks
OS: Windows 7 → All
Hardware: x86 → All
Summary: use new async Places APIs in Sync → History sync: use mozIAsyncHistory::updatePlaces
Blocks: 622762
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.
No longer blocks: 622762
Depends on: 622762
tracking-fennec: 2.0b4+ → 2.0b5+
Attached patch Preq (v1): Utils.checkGUID (obsolete) — Splinter Review
Attachment #507601 - Flags: review?(rnewman)
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.
Attachment #507603 - Flags: review?(rnewman)
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.
This refactors the history engine to use mozIAsyncHistory::updatePlaces when available. Makes the tests in part 0 pass.
Attachment #507605 - Flags: review?(rnewman)
Whiteboard: [hardblocker] → [hardblocker][has patch][needs review 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.
(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.
For queen and country.
Attachment #507601 - Attachment is obsolete: true
Attachment #507617 - Flags: review?(rnewman)
Attachment #507601 - Flags: review?(rnewman)
Comment on attachment 507617 [details] [diff] [review]
Preq (v2): Utils.checkGUID

Make it so!
Attachment #507617 - Flags: review?(rnewman) → review+
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.
Attachment #507603 - Flags: review?(rnewman) → review+
(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.
(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 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...
Attachment #507605 - Flags: review?(rnewman) → review+
(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.
(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.)
Status: NEW → ASSIGNED
Whiteboard: [hardblocker][has patch][needs review rnewman] → [hardblocker][has patch][has review rnewman]
Address rnewman's review comments from Comment 14.
Attachment #507603 - Attachment is obsolete: true
Address rnewman's review comments:
* Introduce function identity for the filter
* Fix typos
Attachment #507605 - Attachment is obsolete: true
Crossweave tests with latest tinderbox build (which has mozIAsyncHistory::updatePlaces) pass. Just needs bug 622762 to land now, otherwise good to go!
Whiteboard: [hardblocker][has patch][has review rnewman] → [hardblocker][has patch][has review]
Blocks: 629603
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.
Whiteboard: [hardblocker][has patch][has review] → [hardblocker][has patch][has review][fx4-fixed-bugday]
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.