Closed Bug 998163 Opened 7 years ago Closed 7 years ago

8% Tp5 Optimized regression on inbound for most all platforms

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression] p=3 s=it-31c-30a-29b.3 [qa-])

Attachments

(1 file, 2 obsolete files)

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/p-yr923nK3U

I originally thought it was bug 991210 but backing it out didn't help. Turns out it was bug 991111.
Flags: firefox-backlog+
Added p=3 s=it-31c-30a-29b.3
Assignee: nobody → edilee
Attached patch v1 (obsolete) — Splinter Review
Delay the click event listener adding to on mouseover. Some reason adding this window level click listener is causing the tp5o regression... ?
Attachment #8408726 - Flags: review?(adw)
jmaher, any idea why the addition of the window-level "click" event listener (9x) in a preloaded hidden document would cause tp5o to regress 8%? Or who would know how to understand this mysteriousness?
Flags: needinfo?(jmaher)
Attachment #8408726 - Attachment description: sponsored tiles mock v1 → v1
Attached patch v2 (obsolete) — Splinter Review
Adds a page level click listener once. Waiting on talos results https://tbpl.mozilla.org/?tree=Try&rev=3b0b5701f9b6
Attachment #8408726 - Attachment is obsolete: true
Attachment #8408726 - Flags: review?(adw)
Flags: needinfo?(jmaher)
Status: NEW → ASSIGNED
Blocks: tiles-dev
Attachment #8408740 - Flags: review?(adw)
For those following along, the patch removes the code that used to add a top-level click listener for each Site created and instead adds the click listener once. This fixes the performance regression because before when the click listener was added to the node, the listener and references would be cleaned up when the Site's node was removed.
Ed, this is a regression in the memory (main rss / private bytes) as well as responsiveness.  For this to be regressing, I would assume the click listener has a memory hit, if we load 49 pages 25 times each, that could get expensive when talking about a few hundred bytes each.
Comment on attachment 8408740 [details] [diff] [review]
v2

Review of attachment 8408740 [details] [diff] [review]:
-----------------------------------------------------------------

Glad it worked!  r+ with comments below.

::: browser/base/content/newtab/page.js
@@ +19,5 @@
>      // Listen for 'unload' to unregister this page.
>      addEventListener("unload", this, false);
>  
> +    // XXX bug 991111 - Not all click events are correctly triggered when
> +    // listening from the xhtml node, so listen from the xul window and filter

Please mention that this is needed for middle clicks on tiles specifically.

@@ +194,5 @@
>          if (this._mutationObserver)
>            this._mutationObserver.disconnect();
>          gAllPages.unregister(this);
>          break;
>        case "click":

Whereas the click listener added to the sites' nodes only captured event.button == 0 clicks, this one captures all button clicks.  (Try and see.)  I don't know why that is.  So this needs to check button == 0 for the newtab-toggle and button == 0 or 1 for sites.  Then, in Site_onClick, the only case we want to observe middle clicks for is for calling _recordSiteClicked, so the other cases there need to ignore button == 1.
Attachment #8408740 - Flags: review?(adw) → review+
Blocks: 998387
Attached patch for checkinSplinter Review
Adds test for bug 998387
Attachment #8408740 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5c8763cd059d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.