998163 - 8% Tp5 Optimized regression on inbound for most all platforms

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Ed Lee :Mardak]

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.

Comment 1

[Ed Lee :Mardak]

Added p=3 s=it-31c-30a-29b.3

Comment 2

[Ed Lee :Mardak]

Attachment: v1

Delay the click event listener adding to on mouseover. Some reason adding this window level click listener is causing the tp5o regression... ?

Comment 3

[Ed Lee :Mardak]

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?

Comment 4

[Ed Lee :Mardak]

Attachment: v2

Adds a page level click listener once. Waiting on talos results https://tbpl.mozilla.org/?tree=Try&rev=3b0b5701f9b6

Comment 5

[Ed Lee :Mardak]

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.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

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 7

[Drew Willcoxon :adw]

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.

Comment 8

[Ed Lee :Mardak]

Attachment: for checkin

Adds test for bug 998387

Comment 9

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/5c8763cd059d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c8763cd059d