631747 - Minimize DOM manipulation on startup/TabItem-creation

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

bug 588630 comment #32:
> At any rate, it's clear that the slowness is just creating all of the TabItems,
> and that most of that DOM manipulation.
> 
> In particular, DOM3 is where we add the close box and the expander to the div
> (rather than creating them in the .html() statement earlier); I believe this is
> left over from when we had .locked.close and .locked.size to contend with.
> Reintegrating them into the .html() call would be an easy win.
> 
> DOM4 is where we poll the DOM for the padding values. This is the same for
> every TabItem, so it's pointless to do it for everyone; we should do it just
> for the first one, and share that across them all.
> 
> Also in DOM4, we're setting this.bounds to $div.bounds(); that's probably
> entirely unnecessary (leftover from a bygone era), since the div's location
> isn't even set up by then. That line should just go away.

bug 588630 comment #33:

> Moreover, the div that we create for each TabItem, pre-tab-specific info, could
> be put in a DOM DocumentFragment, which can be cloned for each TabItem
> instance. This could possibly be a great performance improvement:
> 
> http://ejohn.org/blog/dom-documentfragments/

We already landed one patch for bug 588630. Opening this bug for these items.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

Using profiling code from bug 588630...

Baseline on my machine (2.4ghz core 2 duo mac, a couple years old), 140-ish tabs:

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 873.00, 873 - (873, 873; 1)
TabItem DOM1 = 2.16, 317 - (1, 17; 147)
TabItem DOM2 = 2.19, 322 - (2, 5; 147)
TabItem DOM3 = 14.53, 2136 - (5, 50; 147)
TabItem DOM4 = 3.43, 504 - (2, 25; 147)
TabItem events = 3.80, 558 - (2, 22; 147)
TabItem reconnect1 = 0.51, 75 - (0, 2; 147)
TabItem reconnect2 = 9.37, 1377 - (7, 137; 147)
TabItem reconnect3 = 8.22, 1209 - (5, 32; 147)
TabItem reconnect4 = 3.14, 462 - (2, 91; 147)
TabItem reconnect5 = 0.88, 129 - (0, 3; 147)
Total = 7962
PANORAMA PROFILE END

With patch:

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 841.00, 841 - (841, 841; 1)
TabItem DOM1 = 2.93, 433 - (1, 156; 148)
TabItem DOM2 = 2.29, 339 - (2, 5; 148)
TabItem DOM3 = 8.11, 1201 - (3, 18; 148)
TabItem DOM4 = 0.31, 46 - (0, 2; 148)
TabItem events = 3.26, 483 - (3, 7; 148)
TabItem reconnect1 = 0.61, 90 - (0, 1; 148)
TabItem reconnect2 = 9.28, 1374 - (7, 106; 148)
TabItem reconnect3 = 0.34, 50 - (0, 30; 148)
TabItem reconnect4 = 4.18, 618 - (0, 99; 148)
TabItem reconnect5 = 0.97, 143 - (0, 2; 148)
Total = 5618
PANORAMA PROFILE END

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1, with profiling code

Incorporated all tasks in the description. Pushed to try to get builds.

Mathnerd, if you can try out the build on your large profile and give us the profiling data again, that would be great!

Comment 3

[Allan Gardner (:Mathnerd314)]

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 67.00, 67 - (67, 67; 1)
TabItem DOM1 = 1.07, 401 - (0, 30; 375)
TabItem DOM2 = 1.31, 493 - (1, 45; 375)
TabItem DOM3 = 18.24, 6841 - (3, 77; 375)
TabItem DOM4 = 14.35, 5381 - (1, 101; 375)
TabItem events = 1.88, 706 - (1, 22; 375)
TabItem reconnect1 = 0.86, 322 - (0, 31; 375)
TabItem reconnect2 = 2.51, 942 - (1, 42; 375)
TabItem reconnect3 = 2.56, 960 - (1, 52; 375)
TabItem reconnect4 = 1.41, 528 - (0, 28; 375)
TabItem reconnect5 = 0.60, 226 - (0, 3; 375)
Total = 16867
PANORAMA PROFILE END 

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 56.00, 56 - (56, 56; 1)
TabItem DOM1 = 1.08, 433 - (0, 8; 400)
TabItem DOM2 = 1.21, 483 - (0, 13; 400)
TabItem DOM3 = 22.55, 9021 - (3, 112; 400)
TabItem DOM4 = 33.62, 13447 - (1, 6372; 400)
TabItem events = 1.89, 755 - (1, 17; 400)
TabItem reconnect1 = 0.84, 335 - (0, 26; 400)
TabItem reconnect2 = 2.59, 1036 - (1, 35; 400)
TabItem reconnect3 = 2.99, 1195 - (1, 132; 400)
TabItem reconnect4 = 1.29, 515 - (0, 26; 400)
TabItem reconnect5 = 0.69, 277 - (0, 15; 400)
Total = 27553
PANORAMA PROFILE END 

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 74.00, 74 - (74, 74; 1)
TabItem DOM1 = 1.13, 504 - (0, 26; 447)
TabItem DOM2 = 1.20, 536 - (1, 17; 447)
TabItem DOM3 = 25.23, 11276 - (3, 122; 447)
TabItem DOM4 = 20.53, 9179 - (1, 142; 447)
TabItem events = 13.21, 5906 - (1, 4971; 447)
TabItem reconnect1 = 0.77, 346 - (0, 14; 447)
TabItem reconnect2 = 2.58, 1154 - (1, 35; 447)
TabItem reconnect3 = 2.69, 1202 - (1, 36; 447)
TabItem reconnect4 = 1.44, 645 - (0, 41; 447)
TabItem reconnect5 = 0.69, 310 - (0, 8; 447)
Total = 31132
PANORAMA PROFILE END 
    
-- AFTER PATCH --

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 96.00, 96 - (96, 96; 1)
TabItem DOM1 = 0.79, 296 - (0, 13; 375)
TabItem DOM2 = 1.14, 429 - (0, 19; 375)
TabItem DOM3 = 18.91, 7092 - (2, 116; 375)
TabItem DOM4 = 0.27, 103 - (0, 2; 375)
TabItem events = 2.07, 777 - (1, 37; 375)
TabItem reconnect1 = 0.87, 325 - (0, 47; 375)
TabItem reconnect2 = 2.61, 977 - (1, 43; 375)
TabItem reconnect3 = 2.80, 1050 - (1, 54; 375)
TabItem reconnect4 = 1.37, 512 - (0, 25; 375)
TabItem reconnect5 = 0.58, 217 - (0, 2; 375)
Total = 11874
PANORAMA PROFILE END

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 61.00, 61 - (61, 61; 1)
TabItem DOM1 = 0.79, 317 - (0, 16; 400)
TabItem DOM2 = 1.19, 474 - (0, 8; 400)
TabItem DOM3 = 19.96, 7985 - (2, 96; 400)
TabItem DOM4 = 0.34, 136 - (0, 15; 400)
TabItem events = 1.94, 777 - (1, 20; 400)
TabItem reconnect1 = 0.81, 322 - (0, 8; 400)
TabItem reconnect2 = 2.91, 1165 - (2, 18; 400)
TabItem reconnect3 = 2.67, 1069 - (1, 22; 400)
TabItem reconnect4 = 1.34, 537 - (0, 42; 400)
TabItem reconnect5 = 0.76, 302 - (0, 14; 400)
Total = 13145
PANORAMA PROFILE END

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 59.00, 59 - (59, 59; 1)
TabItem DOM1 = 0.75, 335 - (0, 17; 447)
TabItem DOM2 = 1.15, 515 - (0, 12; 447)
TabItem DOM3 = 22.03, 9846 - (2, 71; 447)
TabItem DOM4 = 0.26, 114 - (0, 4; 447)
TabItem events = 2.04, 914 - (1, 17; 447)
TabItem reconnect1 = 0.74, 330 - (0, 12; 447)
TabItem reconnect2 = 2.59, 1158 - (1, 26; 447)
TabItem reconnect3 = 2.60, 1160 - (1, 21; 447)
TabItem reconnect4 = 1.24, 553 - (0, 25; 447)
TabItem reconnect5 = 0.64, 287 - (0, 11; 447)
Total = 15271
PANORAMA PROFILE END

DOM3 is still doing too much work, but it's definitely a great improvement.

Comment 4

[Allan Gardner (:Mathnerd314)]

Also, the "unresponsive script" dialog cutoff has moved to between 475 and 490 tabs (before it was between 375 and 400)

Comment 5

[Michael Yoshitaka Erlewine [:mitcho]]

Those are great results Mathnerd: a 50% win on startup for profiles with 400+ tabs.

It looks like we're actually spending a good chunk of time in DOM3 just getting the canvas element's getBoundingClientRect in order to get its height and width values. Note, though, that these values are simply the same as offsetHeight and offsetWidth (https://developer.mozilla.org/en/Determining_the_dimensions_of_elements).

Doing some profiling, though, I just realized that actually it doesn't matter whether we use offsetHeight/Width or the getBoundingClientRect method... either way, the first such call can take a little time. Hmm...

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Oh! The width and height computed during TabCanvas.init is *always* going to be 2x2 because the canvas' position is based on the tabs, and the tab has yet to be positioned at this point. The canvas width/height values get overwritten during the first _update instead, after the real bounds are computed. So, we can actually just not set the width/height on TabCanvas.init...

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2, with profiling code

Removed TabCanvas.init, as it wasn't really doing anything (as noted above) and was just slowing things down. _update will trigger the correct setting of width and height values on the <canvas>.

Sent to try for builds:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=c6cce02545e9

Mathnerd, please give us your profiling results again. It's much appreciated! Thanks! :D

Comment 8

[Allan Gardner (:Mathnerd314)]

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 33.00, 33 - (33, 33; 1)
GroupItems init = 107.00, 107 - (107, 107; 1)
TabItems init = 4.00, 4 - (4, 4; 1)
TabItem DOM1 = 0.81, 402 - (0, 5; 495)
TabItem DOM2 = 1.19, 590 - (1, 10; 495)
TabItem DOM3 = 0.22, 110 - (0, 2; 495)
TabItem DOM4 = 0.25, 124 - (0, 4; 495)
TabItem events = 2.05, 1017 - (1, 77; 495)
TabItem reconnect1 = 0.75, 369 - (0, 15; 495)
TabItem reconnect2 = 2.71, 1329 - (1, 49; 491)
TabItem reconnect3 = 19.38, 9515 - (15, 97; 491)
TabItem reconnect4 = 1.47, 724 - (0, 35; 491)
TabItem reconnect5 = 0.79, 391 - (0, 7; 495)
Total = 14715
PANORAMA PROFILE END

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 5.00, 5 - (5, 5; 1)
GroupItems init = 54.00, 54 - (54, 54; 1)
TabItems init = 4.00, 4 - (4, 4; 1)
TabItem DOM1 = 0.79, 359 - (0, 12; 454)
TabItem DOM2 = 1.12, 507 - (0, 8; 454)
TabItem DOM3 = 0.24, 110 - (0, 5; 454)
TabItem DOM4 = 0.21, 96 - (0, 3; 454)
TabItem events = 1.84, 834 - (1, 11; 454)
TabItem reconnect1 = 0.59, 266 - (0, 22; 454)
TabItem reconnect2 = 2.50, 1136 - (1, 11; 454)
TabItem reconnect3 = 2.57, 1168 - (1, 8; 454)
TabItem reconnect4 = 1.11, 503 - (0, 18; 454)
TabItem reconnect5 = 0.74, 337 - (0, 19; 454)
Total = 5379
PANORAMA PROFILE END

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 5.00, 5 - (5, 5; 1)
GroupItems init = 57.00, 57 - (57, 57; 1)
TabItems init = 4.00, 4 - (4, 4; 1)
TabItem DOM1 = 0.84, 379 - (0, 32; 450)
TabItem DOM2 = 1.11, 500 - (1, 6; 450)
TabItem DOM3 = 0.25, 111 - (0, 4; 450)
TabItem DOM4 = 0.25, 114 - (0, 4; 450)
TabItem events = 1.82, 818 - (1, 18; 450)
TabItem reconnect1 = 0.56, 252 - (0, 6; 450)
TabItem reconnect2 = 2.60, 1170 - (1, 28; 450)
TabItem reconnect3 = 2.53, 1138 - (1, 32; 450)
TabItem reconnect4 = 1.04, 469 - (0, 18; 450)
TabItem reconnect5 = 0.59, 265 - (0, 4; 450)
Total = 5282
PANORAMA PROFILE END

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 4.00, 4 - (4, 4; 1)
GroupItems init = 53.00, 53 - (53, 53; 1)
TabItems init = 3.00, 3 - (3, 3; 1)
TabItem DOM1 = 0.71, 317 - (0, 11; 447)
TabItem DOM2 = 1.11, 496 - (0, 6; 447)
TabItem DOM3 = 0.37, 165 - (0, 48; 447)
TabItem DOM4 = 0.23, 101 - (0, 4; 447)
TabItem events = 1.81, 810 - (1, 22; 447)
TabItem reconnect1 = 0.50, 224 - (0, 5; 447)
TabItem reconnect2 = 2.41, 1077 - (1, 7; 447)
TabItem reconnect3 = 2.53, 1133 - (1, 31; 447)
TabItem reconnect4 = 1.04, 463 - (0, 16; 447)
TabItem reconnect5 = 0.61, 272 - (0, 8; 447)
Total = 5118
PANORAMA PROFILE END

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 5.00, 5 - (5, 5; 1)
GroupItems init = 66.00, 66 - (66, 66; 1)
TabItems init = 4.00, 4 - (4, 4; 1)
TabItem DOM1 = 0.77, 317 - (0, 34; 410)
TabItem DOM2 = 1.35, 552 - (0, 50; 410)
TabItem DOM3 = 0.24, 97 - (0, 3; 410)
TabItem DOM4 = 0.22, 89 - (0, 3; 410)
TabItem events = 1.81, 742 - (1, 35; 410)
TabItem reconnect1 = 0.48, 195 - (0, 5; 410)
TabItem reconnect2 = 2.90, 1189 - (1, 81; 410)
TabItem reconnect3 = 2.62, 1075 - (1, 36; 410)
TabItem reconnect4 = 1.07, 437 - (0, 32; 410)
TabItem reconnect5 = 0.62, 253 - (0, 18; 410)
Total = 5021
PANORAMA PROFILE END

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 5.00, 5 - (5, 5; 1)
GroupItems init = 54.00, 54 - (54, 54; 1)
TabItems init = 3.00, 3 - (3, 3; 1)
TabItem DOM1 = 0.76, 302 - (0, 8; 400)
TabItem DOM2 = 1.07, 430 - (0, 6; 400)
TabItem DOM3 = 0.24, 97 - (0, 3; 400)
TabItem DOM4 = 0.27, 106 - (0, 7; 400)
TabItem events = 1.70, 681 - (1, 7; 400)
TabItem reconnect1 = 0.50, 199 - (0, 5; 400)
TabItem reconnect2 = 2.50, 998 - (1, 13; 400)
TabItem reconnect3 = 2.36, 945 - (1, 9; 400)
TabItem reconnect4 = 1.07, 427 - (0, 17; 400)
TabItem reconnect5 = 0.61, 243 - (0, 4; 400)
Total = 4490
PANORAMA PROFILE END

PANORAMA PROFILE START: average, total - (min, max; count)
UI init = 5.00, 5 - (5, 5; 1)
GroupItems init = 50.00, 50 - (50, 50; 1)
TabItems init = 3.00, 3 - (3, 3; 1)
TabItem DOM1 = 0.66, 249 - (0, 6; 375)
TabItem DOM2 = 1.13, 425 - (1, 10; 375)
TabItem DOM3 = 0.26, 97 - (0, 4; 375)
TabItem DOM4 = 0.22, 84 - (0, 5; 375)
TabItem events = 1.73, 647 - (1, 12; 375)
TabItem reconnect1 = 0.47, 176 - (0, 4; 375)
TabItem reconnect2 = 2.57, 965 - (1, 29; 375)
TabItem reconnect3 = 2.33, 873 - (1, 10; 375)
TabItem reconnect4 = 1.00, 375 - (0, 21; 375)
TabItem reconnect5 = 0.54, 203 - (0, 7; 375)
Total = 4152
PANORAMA PROFILE END

The first one was from a cold startup, so it's probably not worth investigating.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Thanks Mathnerd. Looks like a huge win across the board! Ian, please review.

Comment 10

[Ian Gilman [:iangilman]]

Comment on attachment 510089 [details] [diff] [review]
Patch v2, with profiling code

Great work!

>   // Function: width
>-  // Returns the width of the receiver.
>+  // Returns the width of the receiver, including padding and border.
>   width: function iQClass_width() {
>-    let bounds = this.bounds();
>-    return bounds.width;
>+    return Math.floor(this[0].offsetWidth);
>   },

Just to be clear, this returns the same value it did before; you just updated the comment to make it clear what it was already doing (in addition to now doing it in a more efficient manner), yes?

>+  if (!TabItems.fragment)
>+    TabItems.initFragment();
>+  document.body.appendChild(TabItems.fragment.cloneNode(true));

I think a cleaner interface for fragment would be like so: 

  document.body.appendChild(TabItems.fragment().cloneNode(true));
  
... where fragment() creates it and caches it the first time. 

>   Profile.tag("TabItem DOM2");

Note that this patch is built on my profiling patch; that'll need to be fixed before this lands.

>+  if (Utils.isEmptyObject(TabItems.tabItemPadding)) {
>+    TabItems.tabItemPadding.x = parseInt($div.css('padding-left'))
>+        + parseInt($div.css('padding-right'));
>+  
>+    TabItems.tabItemPadding.y = parseInt($div.css('padding-top'))
>+        + parseInt($div.css('padding-bottom'));
>+  }
>+  this.sizeExtra = TabItems.tabItemPadding;

How about instead of keeping a this.sizeExtra, we just always reference TabItems.tabItemPadding?

>+      Profile.tag("GroupItems init");
>       GroupItems.init();
>       GroupItems.pauseArrange();
>       let hasGroupItemsData = GroupItems.load();
> 
>       // ___ tabs
>+      Profile.tag("TabItems init");

These new profiling calls should be removed as well.

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #10)
> Comment on attachment 510089 [details] [diff] [review]
> >   // Function: width
> >-  // Returns the width of the receiver.
> >+  // Returns the width of the receiver, including padding and border.
> >   width: function iQClass_width() {
> >-    let bounds = this.bounds();
> >-    return bounds.width;
> >+    return Math.floor(this[0].offsetWidth);
> >   },
> 
> Just to be clear, this returns the same value it did before; you just updated
> the comment to make it clear what it was already doing (in addition to now
> doing it in a more efficient manner), yes?

Yup.

> >+  if (!TabItems.fragment)
> >+    TabItems.initFragment();
> >+  document.body.appendChild(TabItems.fragment.cloneNode(true));
> 
> I think a cleaner interface for fragment would be like so: 
> 
>   document.body.appendChild(TabItems.fragment().cloneNode(true));
> 
> ... where fragment() creates it and caches it the first time. 

Done.

> >   Profile.tag("TabItem DOM2");
> 
> Note that this patch is built on my profiling patch; that'll need to be fixed
> before this lands.

Fixed.

> >+  if (Utils.isEmptyObject(TabItems.tabItemPadding)) {
> >+    TabItems.tabItemPadding.x = parseInt($div.css('padding-left'))
> >+        + parseInt($div.css('padding-right'));
> >+  
> >+    TabItems.tabItemPadding.y = parseInt($div.css('padding-top'))
> >+        + parseInt($div.css('padding-bottom'));
> >+  }
> >+  this.sizeExtra = TabItems.tabItemPadding;
> 
> How about instead of keeping a this.sizeExtra, we just always reference
> TabItems.tabItemPadding?

Done. I thought of that too after I made this first diff. :)

> >+      Profile.tag("GroupItems init");
> >       GroupItems.init();
> >       GroupItems.pauseArrange();
> >       let hasGroupItemsData = GroupItems.load();
> > 
> >       // ___ tabs
> >+      Profile.tag("TabItems init");
> 
> These new profiling calls should be removed as well.

Done.

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v3, no profiling code

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Hmm, odd, try failed just on osx debug, on this test:

browser/base/content/test/tabview/browser_tabview_bug626525.js

http://tbpl.mozilla.org/?tree=MozillaTry&rev=4dca2c90508b

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v3.1

Thanks for the r+ Ian.

Unfortunately, the test failures, while only on osx debug, were legit and I could reproduce them locally. The problem was with the bug 626525 patch: it would issue the "move to new tab group" command (which triggers an _initFrame with that "move to new tab group" code in the callback, using DOMContentLoaded) and also add a callback to tabviewframeinitialized which triggers toggle. Depending on the precise timing of the firing of DOMContentLoaded and its "move to new tab group" code and the firing of tabviewframeinitialized, this would create a race condition. Apparently speeding up the Panorama init code made this latent race condition apparent.

Here's what I've done in this new patch to fix this:

1. Changed the _initFrame callback to use tabviewframeinitialized instead of DOMContentLoaded. Thus both callbacks (the "move to new tab group" code and the toggle) in the bug 626525 test simply follow the general order of listener firing.
2. This change, in turn, made apparent a similar race condition using DOMContentLoaded in test browser_tabview_startup_transitions.js (bug 591705). This was alleviated by tweaking the test logic to start the meat of its testing on load instead of DOMContentLoaded.

Passes locally, sent to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=84fa50be0e68

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 511299 [details] [diff] [review]
Patch v3.1

+ Gavin for quick review of the test/race condition fix in patch 3.1, in case he gets to it before Ian. Gavin, note that patch v3 received r+ from Ian... the only changes are in browser/base/content/browser-tabview.js and browser/base/content/test/tabview/browser_tabview_startup_transitions.js, as explained in the previous comment.

Comment 16

[Ian Gilman [:iangilman]]

Comment on attachment 511299 [details] [diff] [review]
Patch v3.1

>+      this._deck.appendChild(iframe);
>+      this._window = iframe.contentWindow;
>+
>       if (typeof callback == "function")
>-        iframe.addEventListener("DOMContentLoaded", callback, false);
>+        window.addEventListener("tabviewframeinitialized", callback, false);
> 
>       iframe.setAttribute("src", "chrome://browser/content/tabview.html");
>-      this._deck.appendChild(iframe);
>-      this._window = iframe.contentWindow;

I'm down with the event change, but why the reordering of the sequence?

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #16)
> Comment on attachment 511299 [details] [diff] [review]
> Patch v3.1
> 
> >+      this._deck.appendChild(iframe);
> >+      this._window = iframe.contentWindow;
> >+
> >       if (typeof callback == "function")
> >-        iframe.addEventListener("DOMContentLoaded", callback, false);
> >+        window.addEventListener("tabviewframeinitialized", callback, false);
> > 
> >       iframe.setAttribute("src", "chrome://browser/content/tabview.html");
> >-      this._deck.appendChild(iframe);
> >-      this._window = iframe.contentWindow;
> 
> I'm down with the event change, but why the reordering of the sequence?

Sorry, at one point I had changed the listener to be a listener on this._window, which is why I had to rearrange it. Fixing now.

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v3.2

Comment 19

[Ian Gilman [:iangilman]]

Comment on attachment 511434 [details] [diff] [review]
Patch v3.2

Lovely

Comment 20

[Michael Yoshitaka Erlewine [:mitcho]]

Passed try except for one known orange and one leak... requested try rerun on that one platform (linux debug) to make sure it's just an intermittent (bug 633247).

http://tbpl.mozilla.org/?tree=MozillaTry&rev=84fa50be0e68

Comment 21

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #20)
> http://tbpl.mozilla.org/?tree=MozillaTry&rev=84fa50be0e68

Reran linux debug, and it indeed came back perfectly clean. Passed try completely then.

Note to approvers: pretty major perf win in Panorama startup time (baseline in comment 3 and with patch in comment 8)

375 tabs: 16.9s -> 4.1s
400 tabs: 27.5s -> 4.5s
447 tabs: 31.1s -> 5.1s

Also, should fix the latent race condition which is the reason for bug 633155.

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 511434 [details] [diff] [review]
Patch v3.2

massive perf/responsiveness win for a fairly low footprint patch. passed try. a+.

Comment 23

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 24

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/fa4b85882789