636673 - Avoid accessing 'arguments' in code that's called a lot

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Referring to 'arguments' anywhere in a function for the first time will create that object, leaving it behind for GC. Avoid like the plague for anything that gets called a lot, e.g.:

* Sync.js (dealt with by bug 636402)
* Utils.bind2() in engines.js
* BookmarksStore.applyIncoming and _setGUID
* Utils.anno() (nowadays only use by BookmarksStore)

Comment 1

[Philipp von Weitershausen [:philikon]]

* Engine.sync() in engines.js does some weird wrapping.

Comment 2

[Philipp von Weitershausen [:philikon]]

This is a top hit for object creations, thus nom'ing it for blocking Fennec.

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Get rid of a bunch of object creations:

* No longer wrap nearly every engine function with instrumentation code, it's just not necessary because we now log millisecond timestamps . Avoids 3*N 'arguments' allocations.

* Avoid binding newitems.recordHandler to 'this'. Means we have to do the ugly 'let self = this' thing, but it saves us 1*N 'arguments' allocations.

* Get rid of Utils.anno. At this point it was just a DWIM-y way of calling Svc.Annos.{get|set}ItemAnnotation that used arguments.length to determine how it was called. This gets rid of multiple 'arguments' allocations per bookmark.

* While I had to touch every bookmark annotation line anyway, I made all the annotation names consts (bug 559166).

* Get rid of other random references to 'arguments' where it's not necessary.

* Get rid of a bunch of log.debug([...].join(" ") patterns that create arrays for no good reason

Comment 4

[Ed Lee :Mardak]

(In reply to comment #3)
> * Avoid binding newitems.recordHandler to 'this'. Means we have to do the ugly
> 'let self = this' thing, but it saves us 1*N 'arguments' allocations.
Does using native Object.bind not help (in this case / in general)?

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to comment #4)
> (In reply to comment #3)
> > * Avoid binding newitems.recordHandler to 'this'. Means we have to do the ugly
> > 'let self = this' thing, but it saves us 1*N 'arguments' allocations.
> Does using native Object.bind not help (in this case / in general)?

You mean Function.bind? TBH I didn't try, but I bet it allocates a new object too (not for the 'arguments' thing but for the rebound function). In any case it's not available on Firefox 3.5/3.6 *cringe*. At this point the 'let self = this' thing is just easier and more predictable.

Comment 6

[Alon Zakai (:azakai)]

(In reply to comment #3)
> * No longer wrap nearly every engine function with instrumentation code, it's
> just not necessary because we now log millisecond timestamps . Avoids 3*N
> 'arguments' allocations.

Just a random thought, it is possible to wrap without using |arguments|, by checking how many parameters the function has, and if it has say less than 5, write 5 arguments manually, so something like

  if (wrapped[name].length <= 5) {
    this[name] = function(arg1, arg2, arg3, arg4, arg5) {
      let start = Date.now();
      try {
        return wrapped[name].call(this, arg1, arg2, arg3, arg4, arg5);

and if more than 5 arguments, use |arguments|.

But of course not wrapping at all is faster :)

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to comment #6)
> Just a random thought, it is possible to wrap without using |arguments|,
<snip>

Oh sure, but this code was utterly unnecessary and just icky. Less code, moar speed! :)

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 516154 [details] [diff] [review]
v1

Plz to be grouping and ordering these:

 const GUID_ANNO = "sync/guid";
-const MOBILE_ANNO = "mobile/bookmarksRoot";
 const PARENT_ANNO = "sync/parent";
+const MOBILEROOT_ANNO = "mobile/bookmarksRoot";
+const SMART_BOOKMARKS_ANNO = "Places/SmartBookmark";
+const DESCRIPTION_ANNO = "bookmarkProperties/description";
+const SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
+const STATICTITLE_ANNO = "bookmarks/staticTitle";
+const FEEDURI_ANNO = "livemark/feedURI";
+const MOBILE_ANNO = "MobileBookmarks";
+const EXCLUDEBACKUP_ANNO = "places/excludeFromBackup";
+const ALLBOOKMARKS_ANNO = "AllBookmarks";
+const SITEURI_ANNO = "livemark/siteURI";
+const GENERATORURI_ANNO = "microsummary/generatorURI";
+const ANNOS_TO_SYNC = [DESCRIPTION_ANNO, SIDEBAR_ANNO, STATICTITLE_ANNO,
+                       FEEDURI_ANNO, SITEURI_ANNO, GENERATORURI_ANNO];
+

Otherwise, delightfully formulaic, and tests pass. Thumbs up.

Comment 9

[Philipp von Weitershausen [:philikon]]

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

Comment 10

[Philipp von Weitershausen [:philikon]]

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

Comment 11

[Philipp von Weitershausen [:philikon]]

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