591711 - We should store column and shouldStack attributes for groups in the session store to speed up tabview display from memory

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

Right now when loading the tab view display, we recompute, on the fly, for each tab which is successively added to each group, (a) how many columns of tabs the group should display and (b) whether the group should just stack its thumbnails.

These attributes could be stored in storage and used verbatim on the first tab view draw.

Of course it's possible for people to greatly change their tab groupings and order before they load , making some of these attributes in storage useless. But using these storage values for starters and then running .arrange() on each group item at the end of reconstitution from memory (just one time each) could be a great improvement.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1, requires 602777 patch first to apply

Comment 2

[Ian Gilman [:iangilman]]

Comment on attachment 481760 [details] [diff] [review]
Patch v1, requires 602777 patch first to apply

I don't think this is really going to work until bug 590268 lands; it kind of depens on us knowing when we're done loading tabs into a group, which we can't really now. I'd recommend shelving it and revisiting at that time (actually shouldn't be too long). For that matter, bug 598795 will probably have a big impact on this; we'll need a lot less reconnect code at that time. 

Once we have instant access to sessionstore, we may not even need to save the stacked/columns info in storage. Instead, just don't arrange a group at all until all of its tabs have been added.

Given the above, I don't know of what use my specific comments will be, but here goes:

+  this._columns = options.columns || 1;

Shouldn't this be 0 instead of 1 (especially since you're using it to flag dontRecomputeStacking)?

-  //   force - true to always update the DOM even if the bounds haven't changed; default false
+  //   force - true to always update the DOM even if the bounds haven't changed;
+  //     default false
   setBounds: function GroupItem_setBounds(rect, immediately, options) {
   
Should add dontRecomputeStacking to the list of options.

   if (!TabItems.reconnect(this))
-    GroupItems.newTab(this);
+    GroupItems.newTab(this, {dontRecomputeStacking: true});

Seems like dontRecomputeStacking should always be false for unconnected tabs (unless they'll be connected later, but we can't know that here, and at any rate, bug 590268 will fix that).

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #2)
> Comment on attachment 481760 [details] [diff] [review]
> Patch v1, requires 602777 patch first to apply
> 
> I don't think this is really going to work until bug 590268 lands; it kind of
> depens on us knowing when we're done loading tabs into a group, which we can't
> really now. I'd recommend shelving it and revisiting at that time (actually
> shouldn't be too long). For that matter, bug 598795 will probably have a big
> impact on this; we'll need a lot less reconnect code at that time. 

This patch (together with the patch for bug 602777) was designed so that we gain improvements on startup by not having to compute and recompute the column values, even if tabs have not been placed into their appropriate groups yet (i.e., pre-590268-fix). If you have a test profile with multiple groups with multiple children in each, you can see the difference without 590268.

If you do this, you'll also notice that the single active group where, on current pre-590268 startup, all our tabs start in, does not explode because it has a preset columns value: that's because our columns value is used as a lower-bound, or seed value. 602777 comment 3 addresses that.

> Once we have instant access to sessionstore, we may not even need to save the
> stacked/columns info in storage. Instead, just don't arrange a group at all
> until all of its tabs have been added.

Then we'd have to store information on the number of children we're expecting, and if for some reason some tab within that group gets removed before loading Panorama, it'd be in some weird state. I think storing a lower-bound for the columns value, as I've done here, and not recomputing the columns value *only on reconstitution*, makes sense.

Even once we have 590268 landed, I think we're going to want this patch.

> Given the above, I don't know of what use my specific comments will be, but
> here goes:
> 
> +  this._columns = options.columns || 1;
> 
> Shouldn't this be 0 instead of 1 (especially since you're using it to flag
> dontRecomputeStacking)?

I'm not sure why this should be 0... I guess this default value doesn't actually matter, but if we're not stacking, it should always be at least 1 anyway... why would you prefer 0?

>    if (!TabItems.reconnect(this))
> -    GroupItems.newTab(this);
> +    GroupItems.newTab(this, {dontRecomputeStacking: true});
> 
> Seems like dontRecomputeStacking should always be false for unconnected tabs
> (unless they'll be connected later, but we can't know that here, and at any
> rate, bug 590268 will fix that).

That sounds right. Removed.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2, requires 602777 patch first to apply

Incorporated a couple (but not all) comments. Also using dontRecomputeStacking when repositioning (but not resizing) a group.

Please reconsider as a patch orthogonal to bug 590268. A lot of the pain in this area will be alleviated with bug 590268... but this patch can make things even more efficient.

Comment 5

[Ian Gilman [:iangilman]]

Comment on attachment 482063 [details] [diff] [review]
Patch v2, requires 602777 patch first to apply

Using it in reorderTabItemsBasedOnTabOrder and setTopChild are nice touches. Unfortunately, those seem to be the only valid uses at the moment: 

* Using it in setBounds is redundant, as we already don't arrange when you're just moving. 

* We can't use it in reconnect, because we're actually adding a new tab, so it may actually need to change columns/stacking. 

* It's useless to use it when creating the group, since nothing will have been reconnected yet. 

Those last two stem from the fact that we can't know ahead of time how many tabs will be in the group (until instant sessionstore is sorted out).

Given that, this seems like pretty small potatoes for now. I still think we should revisit this after instant sessionstore.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #5)
> Given that, this seems like pretty small potatoes for now. I still think we
> should revisit this after instant sessionstore.

Alright, let's do that.

Comment 7

[Ian Gilman [:iangilman]]

(In reply to comment #3)

Sorry, didn't see this earlier; replying to the points raised. 

> > Once we have instant access to sessionstore, we may not even need to save the
> > stacked/columns info in storage. Instead, just don't arrange a group at all
> > until all of its tabs have been added.
> 
> Then we'd have to store information on the number of children we're expecting,
> and if for some reason some tab within that group gets removed before loading
> Panorama, it'd be in some weird state. I think storing a lower-bound for the
> columns value, as I've done here, and not recomputing the columns value *only
> on reconstitution*, makes sense.

We wouldn't have to store how many children we were expecting; instead we'd just loop through all the tabs before arranging anything. At that point we would know how many tabs were in each group.

> Even once we have 590268 landed, I think we're going to want this patch.

Agreed; I just think it can be a good deal different at that point. 

> > +  this._columns = options.columns || 1;
> > 
> > Shouldn't this be 0 instead of 1 (especially since you're using it to flag
> > dontRecomputeStacking)?
> 
> I'm not sure why this should be 0... I guess this default value doesn't
> actually matter, but if we're not stacking, it should always be at least 1
> anyway... why would you prefer 0?

In bug 602777 you make options.columns optional; if it's falsy, you use one for the colums value. Using a value of zero for this._columns works well with that, and furthermore is semantically correct: it means we don't know how many columns, rather than saying we think we have one column.

So yeah, not really a big deal... just seemed like the thing to do.

Comment 8

[Kevin Hanes]

Moving to b9

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Ian, Sean, what do you think is the status of related components and/or the necessity of this task at this point in time? You surely have kept abreast of related components recently better than I have.

Here's my thought:

The main place where this suggestion originally made sense was at the loading-up-from-session-store-on-init stage, before we had the immediate session store access. It let us avoid the recalculation of the number of columns required for each tab addition to the group. Even though we can add all tabs to their correct parents immediately, I believe we could still incorporate this patch to, again, alleviate all that excess computation on panorama startup.

Comment 10

[Ian Gilman [:iangilman]]

I believe bug 588630 (which has already landed), possibly together with bug 598795 (which hasn't yet landed) makes this obsolete; it pauses arrangement until after all of the tabs have been loaded.