Closed
Bug 636305
Opened 13 years ago
Closed 13 years ago
History engine: make incoming records comply to mozIPlaceInfo
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0+ | --- |
People
(Reporter: philikon, Assigned: philikon)
References
Details
Attachments
(1 file, 1 obsolete file)
8.47 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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.
Comment 2•13 years ago
|
||
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-
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
This is a top hit for object creations, thus nom'ing it for blocking Fennec. Will have a new patch later today.
tracking-fennec: --- → ?
Updated•13 years ago
|
tracking-fennec: ? → 2.0+
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [has patch][needs review rnewman]
Comment 6•13 years ago
|
||
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+
Updated•13 years ago
|
Whiteboard: [has patch][needs review rnewman] → [has patch][has review]
Assignee | ||
Comment 7•13 years ago
|
||
Pushed to fx-sync: https://hg.mozilla.org/services/fx-sync/rev/f17d96ae08b7
Whiteboard: [has patch][has review] → [fixed in fx-sync]
Assignee | ||
Comment 8•13 years ago
|
||
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]
Assignee | ||
Comment 9•13 years ago
|
||
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/2b14cbd242f2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-sync][fixed in services]
Comment 10•13 years ago
|
||
History sync working fine on stage-auth with beta12
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 11•13 years ago
|
||
(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: 13 years ago → 13 years ago
Updated•6 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.
Description
•