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.

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

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!

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.

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

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...

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...

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

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.

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

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.

(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.

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

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 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 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?

(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 on attachment 511434 [details] [diff] [review] Patch v3.2 Lovely

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

(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 on attachment 511434 [details] [diff] [review] Patch v3.2 massive perf/responsiveness win for a fairly low footprint patch. passed try. a+.