Closed Bug 636305 Opened 9 years ago Closed 9 years ago

History engine: make incoming records comply to mozIPlaceInfo

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: philikon, Assigned: philikon)

References

Details

Attachments

(1 file, 1 obsolete file)

We're spending a lot of time GC'ing during history sync. This may be due to the fact that we create a bunch of new objects for each incoming record. We should try to reuse the existing objects as much as possible.
Attached patch v1 (obsolete) — Splinter Review
Change the history engine to do as much in-place as possible:

* The list of HistoryRecs and their visits are modified in-place (look out for array.splice... fun!) rather than using Array.filter. This also avoids creating a function object on each invocation.

* To de-dupe history visits, the query result is rewritten so that array.indexOf can be used to find dupes, rather than building a hash table.

* Incoming HistoryRec objects, including their visit objects, are modified to comply to the mozIPlaceInfo interface, rather than creating new objects.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #515786 - Flags: review?(rnewman)
Comment on attachment 515786 [details] [diff] [review]
v1

No Array.splice in a tight loop -- return value is an allocated array.

Use chasing pointers to pack as we process, then truncate at the end.
Attachment #515786 - Flags: review?(rnewman) → review-
Also:

+    for (let i = 0; i < curVisits.length; i++) {
+      curVisits[i] = curVisits[i].date + "," + curVisits[i].type;
     }


Would be worthwhile experimenting with a version of getVisits (getVisitStrings?) which does this concatenation for you.
This is a top hit for object creations, thus nom'ing it for blocking Fennec. Will have a new patch later today.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Attached patch v2Splinter Review
Now with 200% less Array.splice(), instead two chasing indices (i, k), rewriting the arrays in place and truncating them at the end.

Kills another ~8*N objects.
Attachment #515786 - Attachment is obsolete: true
Attachment #516028 - Flags: review?(rnewman)
Whiteboard: [has patch][needs review rnewman]
Comment on attachment 516028 [details] [diff] [review]
v2

Looks good and tests pass. Comments already mentioned IRL:

* Please comment the two truncations. They're important lines.
* Typo: "// Walk throught he visits".
Attachment #516028 - Flags: review?(rnewman) → review+
Whiteboard: [has patch][needs review rnewman] → [has patch][has review]
Pushed to fx-sync: https://hg.mozilla.org/services/fx-sync/rev/f17d96ae08b7
Whiteboard: [has patch][has review] → [fixed in fx-sync]
Merged to s-c: http://hg.mozilla.org/services/services-central/rev/2b14cbd242f2
Whiteboard: [fixed in fx-sync] → [fixed in fx-sync][fixed in services]
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/2b14cbd242f2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-sync][fixed in services]
History sync working fine on stage-auth with beta12
Status: RESOLVED → VERIFIED
(In reply to comment #10)
> History sync working fine on stage-auth with beta12

Tracy, this just landed on mozilla-central today, not in beta12.
Status: VERIFIED → RESOLVED
Closed: 9 years ago9 years ago
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.