636305 - History engine: make incoming records comply to mozIPlaceInfo

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

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.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

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

[Richard Newman [: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.

Comment 3

[Richard Newman [:rnewman]]

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.

Comment 4

[Philipp von Weitershausen [:philikon]]

This is a top hit for object creations, thus nom'ing it for blocking Fennec. Will have a new patch later today.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: v2

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.

Comment 6

[Richard Newman [: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".

Comment 7

[Philipp von Weitershausen [:philikon]]

Pushed to fx-sync: https://hg.mozilla.org/services/fx-sync/rev/f17d96ae08b7

Comment 8

[Philipp von Weitershausen [:philikon]]

Merged to s-c: http://hg.mozilla.org/services/services-central/rev/2b14cbd242f2

Comment 9

[Philipp von Weitershausen [:philikon]]

Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/2b14cbd242f2

Comment 10

[Tracy Walker [:tracy]]

History sync working fine on stage-auth with beta12

Comment 11

[Philipp von Weitershausen [:philikon]]

(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.