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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 months ago

People

(Reporter: philikon, Assigned: philikon)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

Attachments

(1 attachment)

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)
* Engine.sync() in engines.js does some weird wrapping.
This is a top hit for object creations, thus nom'ing it for blocking Fennec.
tracking-fennec: --- → ?

Updated

8 years ago
tracking-fennec: ? → 2.0+
Assignee: nobody → philipp
Blocks: 559166
Posted patch v1Splinter Review
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
Attachment #516154 - Flags: review?(rnewman)

Comment 4

8 years ago
(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)?
(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.
(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 :)
(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 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.
Attachment #516154 - Flags: review?(rnewman) → review+
Pushed to fx-sync: https://hg.mozilla.org/services/fx-sync/rev/00aff7b777bf
Status: NEW → ASSIGNED
Whiteboard: [fixed in fx-sync]
Merged to s-c: http://hg.mozilla.org/services/services-central/rev/bf00bb0056ce
Whiteboard: [fixed in fx-sync] → [fixed in fx-sync][fixed in services]
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/bf00bb0056ce
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-sync][fixed in services]
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.