At the moment, stacks have thumbnails for all tabs, regardless of whether you can actually see them or not. To help with performance issues, only the frontmost in the stack should have a thumbnail. The rest of the stack can either have blank thumbnails, or (ideally) just a static image that makes it look like there are many tabs behind it.

Note that this still applies even if bug 597258 gets fixed.

Part of the performance meta project.

We may still want to generate the thumbnails (so they'll be immediately available if/when the user expands the stack), but prioritize them below all visible thumbnails.

Another option is to chunk up the stack into a canvas as an optimization. This way not only do you not have to keep all of those obscured tabs updating, but dragging a stack will get much faster as only one object is actually being dragged.

I'm going to solve this bug by putting tabs in a stack on the end of the work queue, and we'll leave groupItem imposter images (bug 604120) as a separate task.

Solved by splitting tab updates into two classes: those that are hidden, and those visible in an arrangement, or at the top of stacks. Hidden tabs get pushed onto the back of the queue, while visible tabs are pushed to the front, and get priority.

Comment on attachment 483099 [details] [diff] [review] v1 > inStack: function TabItem_inStack() { > return iQ(this.container).hasClass("stacked"); > }, I know this isn't part of your patch, but in terms of performance (and correctness) we shouldn't be talking to the DOM any more than we have to. This should instead be: return this.parent && this.parent.isStacked(); This implies adding an isStacked method to return the "private" _isStacked property of GroupItem. >+ // Returns true if this item is showing on top of the stack, >+ // determined by whether this tab is its parents topChild, or >+ // if it doesn't have one, its first child. >+ topOfStack: function TabItem_topOfStack() { >+ return this.inStack() && ((this.parent.topChild == this) || >+ (!this.parent.topChild && this.parent.getChild(0) == this)); >+ }, In terms of encapsulation, seems like the TabItem should ask the GroupItem whether it's the top child of a stack; that way we don't have to remember to change TabItem if GroupItem's logic changes. Not a big deal, I suppose, for something as simple as this, but still worth considering. >- this._tabsWaitingForUpdate.push(tab); >+ // Place non-stacked, or top-of-stack tabitems at the head of the >+ // work queue. Everything else goes to the end. >+ if (!tab.tabItem.inStack() || tab.tabItem.topOfStack()) >+ this._tabsWaitingForUpdate.unshift(tab); >+ else >+ this._tabsWaitingForUpdate.push(tab); I'm concerned that this changes a part of our render queue to being LIFO, which would mean if we have enough things clammoring to be updated, the ones at the bottom of the high-priority stack won't get serviced. Probably a better approach would be to have two FIFO queues.

I created a new object for abstracting a two-queue update solution (TabPriorityQueue), as well as the issues you brought up.

Comment on attachment 489567 [details] [diff] [review] v2 I like the new queue object; seems like we're getting there. There are still some issues, perhaps from miscommunication on my part. > update: function TabItems_update(tab) { > try { > Utils.assertThrow(tab, "tab"); > Utils.assertThrow(!tab.pinned, "shouldn't be an app tab"); > Utils.assertThrow(tab.tabItem, "should already be linked"); > > let shouldDefer = ( > this.isPaintingPaused() || >- this._tabsWaitingForUpdate.length || >+ this._tabsWaitingForUpdate.hasItems() || > Date.now() - this._lastUpdateTime < this._heartbeatTiming > ); > > let isCurrentTab = ( > !UI._isTabViewVisible() && > tab == gBrowser.selectedTab > ); > > if (shouldDefer && !isCurrentTab) { >- if (this._tabsWaitingForUpdate.indexOf(tab) == -1) >- this._tabsWaitingForUpdate.push(tab); >- this.startHeartbeat(); >+ this._tabsWaitingForUpdate.push(tab); >+ this.heartbeat(); > } else > this._update(tab); > } catch(e) { > Utils.log(e); > } > }, This call to heartbeat has two issues: * Heartbeat actually does an update, so the whole point of having an update queue (disconnecting update events from update processing so if we get a big spike of events we don't try to process them all at the same time) is subverted. * Heartbeat doesn't do anything if _heartbeatOn is false, nor does it set it to true, so this doesn't kick off the heartbeat if needed. I think you still need something like startHeartbeat for this. > _update: function TabItems__update(tab) { > try { > Utils.assertThrow(tab, "tab"); > >- // ___ remove from waiting list if needed >- let index = this._tabsWaitingForUpdate.indexOf(tab); >- if (index != -1) >- this._tabsWaitingForUpdate.splice(index, 1); We removed tabs from the update queue here not only for the heartbeat, but because it's possible for _update to be called directly from update, and it's possible the tab in question may already be in the queue. Removing it here saves us from a double update. I'd recommend putting the remove back in here (or, I suppose, next to the call to _update in update, if we're confident _update won't be called from anywhere else). >+ // _update will remove the tab from the waiting list >+ let item = this._tabsWaitingForUpdate.pop(); This comment does not speak the truth (though perhaps this will change as per above). >+ push: function TabPriorityQueue_push(tab) { >+ let item = tab.tabItem; >+ if (item.parent && (!item.parent.isStacked() || >+ item.parent.isTopOfStack(item))) { >+ if (this._high.indexOf(tab) == -1) >+ this._high.unshift(tab); >+ } else { >+ if (this._low.indexOf(tab) == -1) >+ this._low.unshift(tab); >+ } >+ }, The priority logic is wrong here. I believe we want everything to go into high unless it's not the top item of a stack, right? It seems like it should be something like so: let isLowPriority = (item.parent && item.parent.isStacked() && !item.parent.expanded); Also, this routine doesn't handle the case where a tab is already in one of the queues but now needs to shift to another (for instance when a group becomes stacked or unstacked). >+ pop: function TabPriorityQueue_pop() { >+ let ret = null; >+ let src = this._high; >+ let alt = this._low; >+ if (!this._popToggle) { >+ src = this._low; >+ alt = this._high; >+ } >+ if (src.length) >+ ret = src.pop(); >+ else if (alt.length) >+ ret = alt.pop(); >+ this._popToggle = !this._popToggle; >+ return ret; >+ }, We don't want _popToggle... we want everything in the high-priority queue to be processed before anything in the low-priority queue. Since all we're putting in the low-priority queue are tabs that are obscured, it's fine if they never get processed at all.

The last patch I attached confused things a bit because I don't think I properly applied dependency 587231 first. So, that should resolve the issues with heartbeat(). Low priority now follows item.parent && (item.parent.isStacked() && !item.parent.isTopOfStack(item) && !item.parent.expanded everything else goes to top, and tabs now switch queues if required. If they already exist in the destination queue, they stay where they are. I had put in popToggle because I thought I had remembered that you didn't want any way for the high priority queue to keep low priority items from updating. I changed this back to a simple high, then low order. I also put in a peek(), so that the next item on the queue can be sent to _update, and removed there.

Comment on attachment 490185 [details] [diff] [review] v3 (In reply to comment #10) > The last patch I attached confused things a bit because I don't think I > properly applied dependency 587231 first. So, that should resolve the issues > with heartbeat(). Much better! > Low priority now follows > item.parent && (item.parent.isStacked() && > !item.parent.isTopOfStack(item) && > !item.parent.expanded Perfect... I forgot isTopOfStack, didn't I? > everything else goes to top, and tabs now switch queues if required. If they > already exist in the destination queue, they stay where they are. Excellent. > I had put in popToggle because I thought I had remembered that you didn't want > any way for the high priority queue to keep low priority items from updating. I > changed this back to a simple high, then low order. What I was concerned about was things in the high-priority queue never getting processed if it was LIFO. Sorry about the confusion. Looks great, though you still have the popToggle property... might as well kill that too. > I also put in a peek(), so > that the next item on the queue can be sent to _update, and removed there. Cool, thanks. Also, I'm now the Panorama module owner, so I can officially review! :)

So, this patch needs approval and try, but otherwise should be good to go.

Unbitrotted Try: http://hg.mozilla.org/try/pushloghtml?changeset=734aeba49d07

(In reply to comment #13) > Created attachment 490278 [details] [diff] [review] > v3 > > Unbitrotted > Try: http://hg.mozilla.org/try/pushloghtml?changeset=734aeba49d07 Was the try successful? I can't tell from that link.

Try failed, due to 587231 test fail. Fixing now.

Comment on attachment 490278 [details] [diff] [review] v3 Canceling approval request, as this is now a blocker

Beta8 has 1 bug left on it, moving our blockers to b9

Not common enough perf to block.

bugspam (moving b9 to b10)

bugspam (removing b9)

I'm confused... this is R+ but we never requested approval because of the temporary blocker-ness confusion?

(In reply to comment #21) > I'm confused... this is R+ but we never requested approval because of the > temporary blocker-ness confusion? Probably, plus the fact that there was a failing test (comment 15). Needs to be unrotted probably, and run through try again. Sounds like mitcho's on that.

There was some minor patch rot. Unrotted, and passed all tests locally. Pushed to try. Rerequesting approval.

Comment on attachment 505619 [details] [diff] [review] v3, unrotted Failed try: Just one failure, consistent across all platforms: browser_tabview_bug595560.js | The search result tab is shown - Got [object XULElement @ 0x7ddd790 (native @ 0x9f4b030)], expected [object XULElement @ 0x70fe760 (native @ 0x8ec1760)]

bugspam. Moving b10 to b11

bugspam. Removing b10

Pushed to try. I believe the error in that last try push was due to another bug on trunk... pushing again to check.

Comment on attachment 510158 [details] [diff] [review] Patch v3, unrotted again february 6 Passed try modulo one known intermittent orange: http://tbpl.mozilla.org/?tree=MozillaTry&rev=312fd3d96ec5 Requesting approval

Comment on attachment 510158 [details] [diff] [review] Patch v3, unrotted again february 6 This is pretty big and doesn't have any tests (not sure if it really is testable either...). I'm inclined to punt on this.

Sean, could you put a quick test together for this? If not, I could try to. Shawn, this patch could have some minimal perf win and also gives us some nice infrastructure to use in bug 627237 (not blocking, but a recommendation from our security review). Do you think we can get a+ on this if we put a test together in the next day or two?

(In reply to comment #30) > Shawn, this patch could have some minimal perf win and also gives us some nice > infrastructure to use in bug 627237 (not blocking, but a recommendation from > our security review). Do you think we can get a+ on this if we put a test > together in the next day or two? I would feel much more comfortable doing so with a test.

Sean, I started looking into writing a test for this, but then realized (searching for references to isTopOfStack) that it looks like this patch right now makes non-top-of-stack tabs have a lower priority in the update queue, but not actually not updating them at all. This doesn't match the description of this bug. Am I missing something in the code and misreading?

Tabs which are in a stack, but not the top, get put into the low priority queue. When update() is called on them, their state may change, and their priority may change. Tabs in the low priority queue will be updated (via calling peek() to find the next one) once the high priority queue tabs are exhausted.

(In reply to comment #33) > Tabs which are in a stack, but not the top, get put into the low priority > queue. When update() is called on them, their state may change, and their > priority may change. Tabs in the low priority queue will be updated (via > calling peek() to find the next one) once the high priority queue tabs are > exhausted. Yes, this was roughly my understanding... in other words, the description here "Stacks should not generate thumbnails for anything but the front-most tab" is not actually what this patch does. I just want to confirm that. Am I missing something?

Yes, the spec for the bug changed with Ian's comment #3. Title changed.

Sean, could you write a test for this?

Ok. The test will ensure that rear tabs get updated, but only after front tabs.

Added test, as well as a small test support member to TabItem. It lets tests hook in a function to be called whenever a tab item updates.

Comment on attachment 511256 [details] [diff] [review] v5 (test added 2010/2/9, debug line removed) (Tim: you are flagged for feedback to take a quick look at createGroupItemWithTabs. Thanks. :)) >+ // ___ testing support. Only tests may set this to a function to be called >+ // when the tab is updated. It's returned to null after calling. >+ this._updateTestFunc = null; Not a huge fan of this kind of hook, myself, but I guess it's legit... it's for Ian to decide, ultimately, though. > Utils.assertThrow(tab._tabViewTabItem, "should already be linked"); >- >+ You added space here. ^^^ >+ // ___ testing support >+ if (tabItem._updateTestFunc) { >+ tabItem._updateTestFunc(tabItem); >+ tabItem._updateTestFunc = null; >+ } I think, for this to be a (potentially) more useful hook for future tests, we let the test unregister the updateTestFunc itself, rather than null-ing it out here. >+++ b/browser/base/content/test/tabview/browser_tabview_bug602432.js >@@ -0,0 +1,140 @@ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 Please use the new public domain header: bug 629514 > let groupItem = createGroupItemWithTabs(contentWindow, 150, 150, 100, > numTabs, true); Why are you calling createGroupItemWithTabs with animate = true? Especially important because you call mainTestFunc right after that, without executeSoon. Try it with animate = false. > // Define main test function > // > let mainTestFunc = function() { There's no point in encapsulating this as a function if you're calling it right after, right? >+ // Force updates to be deferred >+ contentWindow.TabItems.pausePainting(); So, a number of operations like setBounds or zoomIn will resumePainting. Is this safe here? I.e., is it going to be reliably paused throughout the test duration? >+ let testFunc = function(tabItem) { >+ if(--leftToUpdate==0) nit: if > 0, return, instead of tab drift. { can go after the control statements, too (iirc). Nice test logic. >+ for(let c=0; c<children.length; ++c) >+ { >+ children[c]._updateTestFunc = testFunc; >+ contentWindow.TabItems.update(children[c].tab); >+ } Right now in head.js's afterAllTabItemsUpdated we call update on each of them, essentially bypassing the TabPriorityQueue. Adding this kind of hook to check that they're all updated and then calling update() on them is much more graceful. Please file a followup to rewrite afterAllTabItemsUpdated in terms of this kind of TabPriorityQueue-obeying logic, and making sure all the tests still pass. (This bug should block bug 585689.) >+function createGroupItemWithTabs(contentWindow, width, height, padding, numNewTabs, animate) { >+ let newItem = contentWindow.gBrowser.loadOneTab("about:blank")._tabViewTabItem; Sorry, does contentWindow have a gBrowser property here? Because it's chrome maybe? Just checking. createGroupItemWithTabs is nice. Let's put this in head.js. Flagging Tim who's written a number of our head.js functions for feedback on this function. >+function closeGroupItem(groupItem, callback) { >+ groupItem.addSubscriber(groupItem, "groupHidden", function() { >+ groupItem.removeSubscriber(groupItem, "groupHidden"); >+ groupItem.addSubscriber(groupItem, "close", function() { >+ groupItem.removeSubscriber(groupItem, "close"); >+ >+ callback(); >+ >+ }); >+ group.closeHidden(); What is this group here? Shouldn't it be groupItem? Also, there's extra whitespace there. Thanks!

(In reply to comment #40) > So, a number of operations like setBounds or zoomIn will resumePainting. Is > this safe here? I.e., is it going to be reliably paused throughout the test > duration? pause/resumePainting are re-entrant. > Please file a followup to rewrite afterAllTabItemsUpdated in terms of > this kind of TabPriorityQueue-obeying logic, and making sure all the tests > still pass. (This bug should block bug 585689.) Bug 633096 > Sorry, does contentWindow have a gBrowser property here? Because it's chrome > maybe? Just checking. It may, but I've changed it to use win.gBrowser now to match the other tests.

Comment on attachment 511307 [details] [diff] [review] v6 (with test and corrections) >+ // Set up tab priority queue >+ this._tabsWaitingForUpdate = new TabPriorityQueue(); >+ nit: space >+ >+ // ___ testing support nit: space >+ if (tabItem._updateTestFunc) { >+ tabItem._updateTestFunc(tabItem); >+ } nit: drop the {} >+++ b/browser/base/content/test/tabview/browser_tabview_bug602432.js >+++ b/browser/base/content/test/tabview/head.js nit: you've got (a little) more trailing whitespace in these files too. f+ from me. Just make sure to make those changes in the next revision. feedbacking Tim again for createGroupItemWithTabs, in case he notices anything.

Comment on attachment 511307 [details] [diff] [review] v6 (with test and corrections) +function createGroupItemWithTabs(win, width, height, padding, numNewTabs, animate) { + let contentWindow = win.document.getElementById("tab-view").contentWindow; Maybe we could use win.TabView.getContentWindow() here? That is a lot more flexible if we're changing how panorama is structured later. + // add blank items + contentWindow.GroupItems.setActiveGroupItem(groupItem); + for(let t=0; t<numNewTabs; t++) { Nit: please add a space here: "for (let t..." + for(let t=0; t<numNewTabs; t++) { + let newItem = win.gBrowser.loadOneTab("about:blank")._tabViewTabItem; + ok(newItem.container, "Created element "+t+":"+newItem.container); + } I could use that function in _many_ tests but we would definitely need some way to specify what urls we want to be loaded. So maybe instead of numNewTabs that could be an array with urls? We could then create a convenience function like createGroupItemWithBlankTabs() that accepts a "numNewTabs" argument? Looks good overall, but I think we should make that bit more flexible (that's a very useful helper function :). And closeGroupItem() is a good catch, too!

Comment on attachment 511307 [details] [diff] [review] v6 (with test and corrections) Forgot to f+ :)

(In reply to comment #43) > And closeGroupItem() is a good catch, too! I've actually wondered whether GroupItem_close should take an argument which means "right now, with no undo". It could be useful for tests, but also our future API. That's another bug, though. ;)

(In reply to comment #45) > I've actually wondered whether GroupItem_close should take an argument which > means "right now, with no undo". It could be useful for tests, but also our > future API. That's another bug, though. ;) +1 That would be great :)

head.js now has a createGroupItemWithTabs( url array) and createGroupItemWithBlankTabs( number of tabs). I fixed most of the spacing nits, except where it appeared that spacing was the style laid down in that block previously. Personally, I think that spacing improves readability.

Regarding bug 633096 and this patch: wouldn't it be better to send a subscriber notification like "update" when a TabItem gets updated? >+ // ___ testing support >+ if (tabItem._updateTestFunc) >+ tabItem._updateTestFunc(tabItem); could be changed to: tabItem._sendToSubscribers("update"); IMHO that's a bit clearer and would be nice to have as an API to attach to tabItem updates. What do you think? :)

(In reply to comment #47) > I fixed most of the spacing nits, except where it appeared that spacing was the > style laid down in that block previously. Personally, I think that spacing > improves readability. My spacing nits were *all* about trailing whitespace. It should not affect readability one bit.

(In reply to comment #48) > >+ tabItem._updateTestFunc(tabItem); > > could be changed to: > > tabItem._sendToSubscribers("update"); > > IMHO that's a bit clearer and would be nice to have as an API to attach to > tabItem updates. What do you think? :) That is indeed the other option... let's have Ian weigh in.

(In reply to comment #50) > That is indeed the other option... let's have Ian weigh in. Done. Far easier to just use the supported feature. (In reply to comment #49) > My spacing nits were *all* about trailing whitespace. It should not affect > readability one bit. Sorry, I mistook trailing whitespace for "extra line breaks".

(removed an errant _updateTestFunc initializer)

Send to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=e1ab158c5be5

Try passed.

This looks like a non-trivial change, which is a concern at this stage. How much of a perf or responsiveness win are we getting? Does this effect perf of Panorama first-show time, or stack painting, or something else?

From comment #30: > Shawn, this patch could have some minimal perf win and also gives us some nice > infrastructure to use in bug 627237 (not blocking, but a recommendation from > our security review). Do you think we can get a+ on this if we put a test > together in the next day or two? So, no, the perf win is not large.

(In reply to comment #56) > From comment #30: > > Shawn, this patch could have some minimal perf win and also gives us some nice > > infrastructure to use in bug 627237 (not blocking, but a recommendation from > > our security review). Do you think we can get a+ on this if we put a test > > together in the next day or two? > > So, no, the perf win is not large. Even if the perf win is not objectively large, it's something. And, more importantly, the user would subjectively think that it's larger, because the tabs that are actually visible would load first, would they not? I would say that it'd be worth the risk of taking it late for the UX improvements it would afford. But that's just my two cents.

Comment on attachment 511423 [details] [diff] [review] v9 After skimming the patch and checking with mitcho, I think this is ok to take.

(In reply to comment #59) > Created attachment 511990 [details] [diff] [review] > v10: runrotted Is there a reason you didn't call this "patch for checkin" and flagged it as such?

Because I forgot. Labeled and flagged.