Closed Bug 579323 Opened 10 years ago Closed 9 years ago

Regression in New Tab animation makes it sluggish

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: aaronmt, Assigned: roc)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase)

Attachments

(10 files, 1 obsolete file)

4.92 KB, image/png
Details
596.62 KB, image/gif
Details
579 bytes, application/vnd.mozilla.xul+xml
Details
4.10 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
23.11 KB, patch
tnikkel
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
12.78 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
12.68 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.11 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
24.82 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Attached image New Tab Animation
Currently between July 15th, 2010 and July 16th, 2010 a checkin has affected the animation of new tabs in that they have become sluggish. One can see the tab title text kind of drag to the right as the animation progresses. July 15th's trunk nightly is good. See screenshot. 

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=24+hours+ago&enddate=now

Tested on: 

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre

and

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100716 Minefield/4.0b2pre
Summary: New tab animation sluggish after → Regression in New tab animation makes it sluggish
Summary: Regression in New tab animation makes it sluggish → Regression in New Tab animation makes it sluggish
I guess bug 564991 did this.

(In reply to comment #0)
> http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=24+hours+ago&enddate=now

That's not a useful range. If you have a good and a bad build, copy the build ids from about:buildconfig and insert them here: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=BAD_BUILD&tochange=GOOD_BUILD
Blocks: 564991
blocking2.0: --- → ?
Component: Tabbed Browser → Layout
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: tabbed.browser → layout
Hardware: x86 → All
(In reply to comment #1)
> That's not a useful range. If you have a good and a bad build, copy the build
> ids from about:buildconfig and insert them here:
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=BAD_BUILD&tochange=GOOD_BUILD

Erm, swap BAD_BUILD and GOOD_BUILD...
This is indeed a regression from retained layers landing: https://bugzilla.mozilla.org/show_bug.cgi?id=564991#c152 . It started happening on the 7th July builds which had the new patches from https://bugzilla.mozilla.org/show_bug.cgi?id=577200 .
blocking2.0: ? → betaN+
Duplicate of this bug: 580734
Blocks: 578785
This should probably block beta 3 (or should have blocked beta 2), as it seems to be a more general problem with -moz-transition which isn't limited to the new-tab animation.
Dao - is there a bug on the moz-transition general bustage? Or are you proposing this bug morph thatward?

Maybe roc can answer that, too?
I don't know. Dao, do you have a testcase for a general problem with -moz-transition?
There's no other bug filed on the problem, this one should suffice. Note that I moved it to Core:Layout when it became clear that retained layers caused this.

I don't have a reduced testcase, but similar artifacts occurred in bug 578785.
Both this bug and bug 578785 deal with things that move and change opacity at the same time. I think that's more likely to be the problem than -moz-transition itself.
Attached file testcase
Keywords: testcase
Nice testcase!
Assignee: nobody → roc
Blocks: 583548
Basically the problem is that XUL layout code calls nsIFrame::Redraw to invalidate the overflow area for the container frame after moving children around, assuming that this will cause all descendants to be fully repainted. But here one of the children has its own ThebesLayer which also needs to be invalidated due to content having moved.

In general we have assumed all over the place that invalidating the overflow rect of a frame when it moves is enough to account for all the descendants of the frame. That is no longer true, now that descendants can have layers that need to be updated.
Blocks: 583878
We might want to proceed with the plan in bug 539356.
Or can we just do something like what bug 581317 did?  Or is that too expensive?
That's going to be expensive unless we optimize it to avoid frame tree traversal, e.g. by adding a NS_FRAME_HAS_DESCENDANT_WITH_CONTAINER_LAYER state bit. And we'd still need to audit callers of Invalidate() to figure out which ones need to traverse those descendants.

Bug 539356 is probably a better plan, but it needs to be revised to deal with layers too.
Bug 539356 is too much for FF4, so instead I'm going to change f->InvalidateOverflowRect to invalidate all ThebesLayer children of ContainerLayers in f's frame subtree.
FrameLayerBuilder::InvalidateThebesLayersInSubtree(nsIFrame* aFrame):
+   * For any descendant frame of aFrame (including across documents) that
+   * has an associated container layer, invalidate all the contents of
+   * all ThebesLayer children of the container. Useful when aFrame is
+   * being moved and we need to invalidate everything in aFrame's subtree.

We call this from nsFrame::InvalidateOverflowRect, assuming that callers are probably moving the frame. The new NS_FRAME_HAS_CONTAINER_LAYER_DESCENDANT bit makes this efficient for the common case where aFrame has no container layers in its descendants.

This patch also moves responsibility for propagating NS_FRAME_HAS_CHILD_WITH_VIEW and NS_FRAME_HAS_CONTAINER_LAYER_DESCENDANT bits on frame reparenting into nsIFrame::SetParent.
Attachment #464017 - Flags: superreview?(dbaron)
Attachment #464017 - Flags: review?(tnikkel)
nsIFrame::Redraw doesn't call nsIFrame::InvalidateOverflowRect. Have it call FrameLayerBuilder::InvalidateThebesLayersInSubtree directly since it's sometimes expected to invalidate descendants.
Attachment #464019 - Flags: review?(dbaron)
Comment on attachment 464017 [details] [diff] [review]
Part 1: Add FrameLayerBuilder::InvalidateThebesLayersInSubtree, call it from InvalidateOverflowRect

The change in meaning of InvalidateOverflowRect is pretty significant, maybe change the name too? InvalidateOverflowRectAndSubtree?

We only use nsSubDocumentFrame::GetSubdocumentRootFrame inside layout, so can we make nsSubDocumentFrame queryframe'able (if it isn't) and just make GetSubdocumentRootFrame a non-virtual function on nsSubDocumentFrame and then call it in nsSubDocumentFrame::BuildDisplayList too?

+#define NS_FRAME_HAS_CONTAINER_LAYER_DESCENDANT     NS_FRAME_STATE_BIT(32)

There is currently a frame state bit at bit 32, and you won't get merge conflicts when you merge. So you'll need to resolve this.

+  if (GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW) {
+    for (nsIFrame* f = aParent;
+         f && !(f->GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW);
+         f = f->GetParent()) {
+      f->AddStateBits(NS_FRAME_HAS_CHILD_WITH_VIEW);
+    }
+  }

It looks like we do not add the NS_FRAME_HAS_CHILD_WITH_VIEW bit to frames that have views, only their parents. So I think this needs to check for the NS_FRAME_HAS_VIEW bit too.

@@ -474,16 +478,18 @@ FrameLayerBuilder::UpdateDisplayItemData
+  } else {
+    props.Set(ThebesLayerInvalidRegionProperty(), new nsRegion());

I think this will set a ThebesLayerInvalidRegionProperty property on every frame that is visible. I think we only want to set it on frames that have container layers.

@@ -1210,20 +1227,21 @@ FrameLayerBuilder::BuildContainerLayerFo
-      // Set up region to collect invalidation data
-      props.Set(ThebesLayerInvalidRegionProperty(), new nsRegion());
+      // The region was deleted to indicate that everything should be
+      // invalidated.
+      state.SetInvalidateAllThebesContent();
     }
-    aContainerFrame->AddStateBits(NS_FRAME_HAS_CONTAINER_LAYER);
+    SetHasContainerLayer(aContainerFrame);
   }

So the idea is that InvalidateThebesLayersInSubtree will be called before a paint, and then during a paint BuildContainerLayerFor is called and detects the deleted ThebesLayerInvalidRegionProperty, and then WillEndTransaction (via UpdateDisplayItemData) restores the ThebesLayerInvalidRegionProperty? Why do we need to move the setting of a new ThebesLayerInvalidRegionProperty from BuildContainerLayerFor to UpdateDisplayItemData?
(In reply to comment #20)
> Comment on attachment 464017 [details] [diff] [review]
> Part 1: Add FrameLayerBuilder::InvalidateThebesLayersInSubtree, call it from
> InvalidateOverflowRect
> 
> The change in meaning of InvalidateOverflowRect is pretty significant, maybe
> change the name too? InvalidateOverflowRectAndSubtree?

Yeah, I should probably call it InvalidateSubtree.

> We only use nsSubDocumentFrame::GetSubdocumentRootFrame inside layout, so can
> we make nsSubDocumentFrame queryframe'able (if it isn't) and just make
> GetSubdocumentRootFrame a non-virtual function on nsSubDocumentFrame and then
> call it in nsSubDocumentFrame::BuildDisplayList too?

OK.

> +#define NS_FRAME_HAS_CONTAINER_LAYER_DESCENDANT     NS_FRAME_STATE_BIT(32)
> 
> There is currently a frame state bit at bit 32, and you won't get merge
> conflicts when you merge. So you'll need to resolve this.

Ah yes! I'll make it 33 of course :-).

> +  if (GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW) {
> +    for (nsIFrame* f = aParent;
> +         f && !(f->GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW);
> +         f = f->GetParent()) {
> +      f->AddStateBits(NS_FRAME_HAS_CHILD_WITH_VIEW);
> +    }
> +  }
> 
> It looks like we do not add the NS_FRAME_HAS_CHILD_WITH_VIEW bit to frames
> that have views, only their parents. So I think this needs to check for the
> NS_FRAME_HAS_VIEW bit too.

Right OK, good catch.

> @@ -474,16 +478,18 @@ FrameLayerBuilder::UpdateDisplayItemData
> +  } else {
> +    props.Set(ThebesLayerInvalidRegionProperty(), new nsRegion());
> 
> I think this will set a ThebesLayerInvalidRegionProperty property on every
> frame that is visible. I think we only want to set it on frames that have
> container layers.

Another good catch!

> @@ -1210,20 +1227,21 @@ FrameLayerBuilder::BuildContainerLayerFo
> -      // Set up region to collect invalidation data
> -      props.Set(ThebesLayerInvalidRegionProperty(), new nsRegion());
> +      // The region was deleted to indicate that everything should be
> +      // invalidated.
> +      state.SetInvalidateAllThebesContent();
>      }
> -    aContainerFrame->AddStateBits(NS_FRAME_HAS_CONTAINER_LAYER);
> +    SetHasContainerLayer(aContainerFrame);
>    }
> 
> So the idea is that InvalidateThebesLayersInSubtree will be called before a
> paint, and then during a paint BuildContainerLayerFor is called and detects the
> deleted ThebesLayerInvalidRegionProperty, and then WillEndTransaction (via
> UpdateDisplayItemData) restores the ThebesLayerInvalidRegionProperty? Why do we
> need to move the setting of a new ThebesLayerInvalidRegionProperty from
> BuildContainerLayerFor to UpdateDisplayItemData?

UpdateDisplayItemData already had the code for clearing the region, so I had the code for setting the region there too. I suppose we can clear the region in BuildContainerLayerFor or set a new region right there.
Oh, we have to preserve the current ThebesLayerInvalidRegionProperty in BuildContainerLayerFor because there might be multiple container layers for the same frame (e.g. transform and opacity) and we need to invalidate the ThebesLayer children of all of them (since we don't know which one will actually have the ThebesLayer children). So we can't clear the region in BuildContainerLayerFor. We might as well clear it, or set it up if there isn't one, in UpdateDisplayItemData.
Attached patch Part 1 v2Splinter Review
Fixed the issues mentioned in comments, except that I'll rename InvalidateOverflowRect in a separate patch, and fix up nsSubDocumentFrame/nsIFrameFrame in separate patches too.
Attachment #464363 - Flags: superreview?(dbaron)
Attachment #464363 - Flags: review?(tnikkel)
I've added a comment to the effect of comment #22 to BuildContainerLayerFor too (just after I uploaded v2).
Breaking the patches down to make it clear what's going on. This only moves the header out.
Attachment #464365 - Flags: review?(tnikkel)
Removes nsIFrameFrame, devirtualizes methods, and also renames nsFrameFrame.cpp to nsSubDocumentFrame.cpp (finally!!! I couldn't resist).

I did not make nsSubDocumentFrame::BuildDisplayList call GetSubdocumentRootFrame. We need subdocView later on in that method anyway so there's no real code simplification available.

At some point we should rename nsSubDocumentFrame to nsSubdocumentFrame...
Attachment #464367 - Flags: review?(tnikkel)
Well, mostly. In a few places we still want InvalidateOverflowRect, so I added that back and renamed the others.
Attachment #464374 - Flags: review?(tnikkel)
I realized that we need to fix nsBlockFrame::ReflowLine to invalidate layers when the line has moved.
Attachment #464376 - Flags: review?(dbaron)
Attachment #464017 - Attachment is obsolete: true
Attachment #464017 - Flags: superreview?(dbaron)
Attachment #464017 - Flags: review?(tnikkel)
I didn't mean that you had to do all that cleanup, but it's nice that you did!
Comment on attachment 464374 [details] [diff] [review]
Part 1.8: Rename InvalidateOverflowRect to InvalidateFrameSubtree

     // Repaint this frame subtree's entire area
-    InvalidateOverflowRect();
+    Invalidate(GetOverflowRectRelativeToSelf());

The comment makes it sound like we want InvalidateFrameSubtree here. If not, why make this change?

There is one InvalidateOverflowRect in nsSVGEffects.cpp and two in nsTableFrame.cpp after your changes where it isn't obvious to me that we don't need InvalidateFrameSubtree.
Attachment #464363 - Flags: review?(tnikkel) → review+
Attachment #464365 - Flags: review?(tnikkel) → review+
Attachment #464366 - Flags: review?(tnikkel) → review+
Attachment #464367 - Flags: review?(tnikkel) → review+
(In reply to comment #31)
> Comment on attachment 464374 [details] [diff] [review]
> Part 1.8: Rename InvalidateOverflowRect to InvalidateFrameSubtree
> 
>      // Repaint this frame subtree's entire area
> -    InvalidateOverflowRect();
> +    Invalidate(GetOverflowRectRelativeToSelf());
> 
> The comment makes it sound like we want InvalidateFrameSubtree here. If not,
> why make this change?

I suspect that only this frame's rendering is affected by selection, but I changed to InvalidateFrameSubtree anyway. It's not worth worrying about.

> There is one InvalidateOverflowRect in nsSVGEffects.cpp and two in
> nsTableFrame.cpp after your changes where it isn't obvious to me that we don't
> need InvalidateFrameSubtree.

The nsTableFrame one was a mistake, changed to InvalidateFrameSubtree. The nsSVGEffects one is in DoUpdate where a clip-path, mask or filter has changed and we need to repaint. Descendant frames are not directly affected.
Attachment #464374 - Flags: review?(tnikkel) → review+
Duplicate of this bug: 585393
Is this issue the same thing that causes the stutter on pages like this one?

http://www.microsoft.com/windows/windows-7/compare/pc-vs-mac.aspx

Click on any of the "having fun", etc. buttons mid page.  The animation is stuttered/sluggish just like the tab animation.
Comment on attachment 464363 [details] [diff] [review]
Part 1 v2

You'll need to use bit 34 now, I think.

Could there be cases where we do something like inserting an entire document that has container layers?  For example, navigating inside an iframe, tab drag-and-drop, etc.?  Or would those always cause new layers to be constructed?  Might there be such cases where you'd need to set the bits?

sr=dbaron
Attachment #464363 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 464019 [details] [diff] [review]
Part 2: actually fix this bug

At some point I think we'll want to honor the damage rect when deciding what layers to invalidate (and what parts of them to invalidate, if that makes sense?).  Though maybe we'll get to display list-based invalidation before we need to.

r=dbaron on this, though
Attachment #464019 - Flags: review?(dbaron) → review+
Comment on attachment 464376 [details] [diff] [review]
Part 3: fix invalidation of lines

r=dbaron
Attachment #464376 - Flags: review?(dbaron) → review+
Though... regarding patch 3:  in what cases do we actually need to invalidate layers there?  Shouldn't the movement of whatever moved move the layers properly, and reflow of whatever's inside them (if any) invalidate their contents appropriately?  Or is the issue that we don't actually do invalidation inside the reflow of some of those frame types because nsBlockFrame covers it up with this code?  (That would explain the need to invalidate layers in the !line->IsBlock() case, but not in the line->IsBlock() case, I think.  Do we really need the first chunk of patch 3?)
Cur(In reply to comment #35)
> Comment on attachment 464363 [details] [diff] [review]
> Part 1 v2
> 
> You'll need to use bit 34 now, I think.

Yep!

> Could there be cases where we do something like inserting an entire document
> that has container layers?  For example, navigating inside an iframe, tab
> drag-and-drop, etc.?  Or would those always cause new layers to be
> constructed?  Might there be such cases where you'd need to set the bits?

I don't think we ever move an existing nsViewportFrame into a new frame/view tree --- well, not currently. So no.

(In reply to comment #36)
> At some point I think we'll want to honor the damage rect when deciding what
> layers to invalidate (and what parts of them to invalidate, if that makes
> sense?).  Though maybe we'll get to display list-based invalidation before we
> need to.

Display-list-based invalidation is the plan.

(In reply to comment #38)
> Though... regarding patch 3:  in what cases do we actually need to invalidate
> layers there?  Shouldn't the movement of whatever moved move the layers
> properly and reflow of whatever's inside them (if any) invalidate their
> contents appropriately?  Or is the issue that we don't actually do
> invalidation
> inside the reflow of some of those frame types because nsBlockFrame covers it
> up with this code?  (That would explain the need to invalidate layers in the
> !line->IsBlock() case, but not in the line->IsBlock() case, I think.  Do we
> really need the first chunk of patch 3?)

The issue is that when a frame has an associated ContainerLayer, (0,0) in the ContainerLayer is the top-left of the toplevel viewport, *not* the top-left of the frame. For ThebesLayers, (0,0) is the top-left of the active scrolled root frame for the ThebesLayer. See FrameLayerBuilder.h.

So when a frame moves, unless it's a scrolled frame being scrolled, the ThebesLayer children need to be repainted. (Transforms are a different story though ... when a transform changes, the frame isn't really moving in frame/display list coordinate space, we just display the buffer somewhere else.)
Whiteboard: [needs landing]
Is this ready for the next beta?
Yes, it's ready to land, just haven't had the cycles to get it landed.
I open the Options dialog and click a link in another app. Then the tab does not animate at all, save for a few px. When I close the Options dialog the tab is properly animated. Is this handled by this bug? Or should I report a new one?
Kai, I'd retest once this bug is fixed, but my suspicion is that you're seeing a different issue.
Apparently this fixes some rendering regressions from bug 130078 - see bug 591671. Marking as a beta5+ blocker.
blocking2.0: betaN+ → beta5+
Blocks: 591671
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b5
Seems to be responsible for a tscroll regression, at least on Linux: http://mzl.la/aTwHfO - we took this for beta5, which I think was the right thing to do, but should figure out if that regression is something we want to monitor.
And OSX - http://mzl.la/d7w93x - adding jrmuizel to help us understand what the test is telling us. Scrolling doesn't *feel* subjectively slower today, but maybe I'm playing with the wrong testcase.
(In reply to comment #43)
> Kai, I'd retest once this bug is fixed, but my suspicion is that you're seeing
> a different issue.

Thanks Boris. I have retested now and I filed bug 592395 on that issue.
Thanks Jeff. There is almost certainly a bug here causing the regression. Where can I get the tscroll test files?
You should be able to grab them out of the talos cvs repo, in the page_load_test/scroll directory
I filed bug 592941 on the regression --- with a patch to fix it.
Depends on: 592941
I landed that patch --- it fixed the regression.
Depends on: 596954
Blocks: 596954
No longer depends on: 596954
Tab animations new/close are still very sluggish if some harder sites are open. :(
Not only tab animations, everything in Firefox is choppy. Take dA for example, Firefox is choppy and unresponsive unil all tabs are fully loaded. But this is *very old* thing.
dA is working for me well, anyway i think there was a JM regression for dA.

On startup, Fx is totally unresponsive until all tabs get fully loadeded... :(
That's what I'm talking about. dA was only an example, this goes for every site, especially for complex ones.
I wonder is there any bugs for this.

I think new tab animation sluggishness and startup issue is two different things.
Don't think so, because both are happening at the same conditions.
Hmm, maybe you are right. File a bug, then.
Bug 593614 is probably our main suspicious.

Share your experiences in that bug, please.
> Share your experiences in that bug, please.

DO NOT spam random unrelated bugs with comments, please.  If you're seeing a problem, just file a bug on it, with steps to reproduce, instead of hijacking other people's bugs.

Steps to reproduce is key.  Comment 54 would make a great bug report if accompanied by the exact list of sites that need to be open to see the problem.
Okay, steps to reproduce:

1. Open a new profile.
2. For being on the safe side, disable Flash player.
3. Open msnbc.com .
4. While it's loading, open a new tab, with the new tab button, and close that tab. Do it several times.

Actual results:

The tab animations are sluggish while the site are loading. When it completes loading, tab animations are smooth again.

Note: this works with every site wich uses little bit more JS. For example while i log into Gmail.
This bug is about a specific regression from bug 564991 that was affecting the tab title animation *all the time* no matter what else was happening.

Please file new bugs on other problems.
Okay, Boris told me to write the steps here.
Filed Bug 599711 .
> Okay, Boris told me to write the steps here.

No, I told you to file a separate bug with the steps.  Thank you for doing that!
Excuse me, i misinterpreted your post.
Comment on attachment 464376 [details] [diff] [review]
Part 3: fix invalidation of lines

This patch adds thebes layer invalidation to two of the three cases in nsBlockFrame::ReflowLine. We invalidate thebes layers in the inline case and the block case if the block moved, but not in the block didn't move but changed size case. Why is it not needed in that case?
Isn't the whole point of the invalidation to handle the case where the child moved?
You need to log in before you can comment on or make changes to this bug.