Closed Bug 773100 Opened 9 years ago Closed 9 years ago

bubblemark redraws the entire page on invalidation on mobile

Categories

(Core :: Layout, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox15 + wontfix
firefox16 + fixed

People

(Reporter: jrmuizel, Assigned: mattwoodrow)

References

(Blocks 1 open bug, )

Details

(Keywords: perf)

Attachments

(1 file)

:(
Blocks: check2
OS: Mac OS X → Android
Hardware: x86 → ARM
Keywords: perf
Does DLBI help here?  (Is it better on one of the nightlies that had DLBI landed?)
So here's a rough idea of what's going on.

PresShell::Paint is being called with a sane region of about 56x54. However, by the time we get down to BasicTiledThebesLayer::InvalidateRegion this has expanded to 480x690.

This happens because CreateOrRecycleThebesLayer() is calling layer->InvalidateRegion(Bounds())
because mInvalidateAllThebesContent is true.

mInvalidateAllThebesContent is set by ContainerSet::SetInvalidateAllThebesContent()

Which is caused by InternalInvalidateThebesLayersInSubtree(nsImageFrame*) deleting the ThebesLayerInvalidRegionProperty()
This was debugged on my single ball bubblemark: http://people.mozilla.org/~jmuizelaar/tests/bubble.html
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Created attachment 642838 [details] [diff] [review]
> Invalidate the frame bounds instead of deleting the frame property

This fixes the problem.
Blocks: 774756
Swift and indiscriminate back-out by ehsan for build failures:
http://hg.mozilla.org/integration/mozilla-inbound/rev/cdef000532ce

tree/layout/base/FrameLayerBuilder.cpp:2253: error: 'InvalidateThebesLayerContents' was not declared in this scope
Duplicate of this bug: 775146
Blocks: 767070
This would hopefully fix bug 767070.
Let's see if this fixes bug 767070 before tracking.
https://hg.mozilla.org/mozilla-central/rev/8a9f2d2b75d4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
bug 767070, comment 24 says this did fix the issue so we'll track this for branches. Please nominate for uplift.
How risky is this?
Comment on attachment 642838 [details] [diff] [review]
Invalidate the frame bounds instead of deleting the frame property

Review of attachment 642838 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is suitable for Aurora but not Beta.
Attachment #642838 - Flags: approval-mozilla-aurora?
Comment on attachment 642838 [details] [diff] [review]
Invalidate the frame bounds instead of deleting the frame property

[Triage Comment]
Approving for Aurora 16. Do you think we can gain confidence in this patch in time to uplift to Beta, or are you suggesting we wontfix already at this point in the cycle? Thanks!
Attachment #642838 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
If mobile really has to have it for beta, I think that's OK. Otherwise I wouldn't push for it.
Depends on: 776836
Brad, Mark, what do you think of this for 15?
I'm still seeing full page invalidating with a recent changset:
https://hg.mozilla.org/mozilla-central/rev/cb0b3e605cf1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 778701
(In reply to JP Rosevear [:jpr] from comment #20)
> Brad, Mark, what do you think of this for 15?

It's a seemingly simple change, but I think we should not uplift for 15. Let it ride.
(In reply to Mark Finkle (:mfinkle) from comment #22)
> (In reply to JP Rosevear [:jpr] from comment #20)
> > Brad, Mark, what do you think of this for 15?
> 
> It's a seemingly simple change, but I think we should not uplift for 15. Let
> it ride.

Given mfinkle and roc's risk evaluation, wontfixing for 15.
Assignee: nobody → matt.woodrow
Depends on: 781414
No longer depends on: 781414
This looks fine on recent nightlies, probably fixed by DLBI.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.