Closed
Bug 1285619
Opened 7 years ago
Closed 7 years ago
We are accumulating ancestor mask layers for PaintedLayers that get optimized to ColorLayers / ImageLayers
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
e10s | + | --- |
firefox47 | --- | unaffected |
firefox48 | --- | unaffected |
firefox49 | --- | fixed |
firefox50 | --- | fixed |
People
(Reporter: mstange, Assigned: botond)
References
Details
(Keywords: regression)
Attachments
(4 files)
No description provided.
Reporter | ||
Comment 1•7 years ago
|
||
STR: 1. Make sure APZ and e10s are on. 2. Turn on display list dumping. 3. Scroll the scrollframe in the testcase. 4. Scroll the root scroll frame in the testcase. You'll see the ColorLayer accumulate a new AncestorMaskLayer with each paint. What's happening here is that we're reusing an existing ColorLayer and not clearing its existing AncestorMaskLayers, and then in ContainerState::SetupScrollingMetadata we add a new ancestor mask layer to the layer's existing ancestor mask layers. There are multiple pieces in the code that make use of existing layers and reset their state; in bug 1267438 we missed two of them: - ownLayer at http://searchfox.org/mozilla-central/rev/f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/base/FrameLayerBuilder.cpp#4085 - ImageLayer / ColorLayer optimized from a PaintedLayer: http://searchfox.org/mozilla-central/rev/f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/base/FrameLayerBuilder.cpp#3197 It would be nice to have all of those pieces of code go through a generic ResetLayerStateForRecycling function.
Blocks: 1267438
status-firefox47:
--- → unaffected
status-firefox48:
--- → unaffected
status-firefox49:
--- → affected
Keywords: regression
Summary: Are we accumulating ancestor mask layers for PaintedLayers that get optimized to ColorLayers / ImageLayers? → We are accumulating ancestor mask layers for PaintedLayers that get optimized to ColorLayers / ImageLayers
Assignee | ||
Comment 2•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63288/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63288/
Attachment #8769345 -
Flags: review?(mstange)
Attachment #8769346 -
Flags: review?(mstange)
Attachment #8769347 -
Flags: review?(mstange)
Assignee | ||
Comment 3•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63290/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63290/
Assignee | ||
Comment 4•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63292/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63292/
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #1) > There are multiple pieces in the code that make use of existing layers and > reset their state; in bug 1267438 we missed two of them: > - ownLayer at > http://searchfox.org/mozilla-central/rev/ > f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/base/FrameLayerBuilder. > cpp#4085 > - ImageLayer / ColorLayer optimized from a PaintedLayer: > http://searchfox.org/mozilla-central/rev/ > f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/base/FrameLayerBuilder. > cpp#3197 I chose call sites slightly upstream from those, because they seemed more appropriate and consistent. Let me know if you agree. ClearExtraDumpInfo() seems like a candidate for calling in ResetLayerStateForRecycling() too - should I add that?
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → botond
Updated•7 years ago
|
tracking-e10s:
--- → ?
![]() |
||
Updated•7 years ago
|
Priority: -- → P2
Reporter | ||
Comment 6•7 years ago
|
||
Comment on attachment 8769345 [details] Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function. https://reviewboard.mozilla.org/r/63288/#review63692 Sorry for taking so long here. Fundamentally, I think the concept of "recycling a layer" should not be part of of the layers API. It's really just something that consumers of the API do with layers, and it's up to them to decide which parts of a layer's state should be reset and which parts should persist. So I think this function should live in FrameLayerBuilder, not on Layer. Maybe just a static function in FrameLayerBuilder.cpp.
Attachment #8769345 -
Flags: review?(mstange)
Reporter | ||
Comment 7•7 years ago
|
||
Comment on attachment 8769346 [details] Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an OwnLayer. https://reviewboard.mozilla.org/r/63290/#review63780
Attachment #8769346 -
Flags: review?(mstange) → review+
Reporter | ||
Comment 8•7 years ago
|
||
Comment on attachment 8769347 [details] Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an image or color layer. https://reviewboard.mozilla.org/r/63292/#review63782
Attachment #8769347 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8769345 [details] Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63288/diff/1-2/
Attachment #8769345 -
Attachment description: Bug 1285619 - Introduce Layer::ResetLayerStateForRecycling(). → Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function.
Attachment #8769345 -
Flags: review?(mstange)
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8769346 [details] Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an OwnLayer. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63290/diff/1-2/
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8769347 [details] Bug 1285619 - Call ResetLayerStateForRecycling() when recycling an image or color layer. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63292/diff/1-2/
Assignee | ||
Comment 12•7 years ago
|
||
Updated to address review comment.
Reporter | ||
Updated•7 years ago
|
Attachment #8769345 -
Flags: review?(mstange) → review+
Reporter | ||
Comment 13•7 years ago
|
||
Comment on attachment 8769345 [details] Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function. https://reviewboard.mozilla.org/r/63288/#review63808
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aa839a7f52f
Comment 15•7 years ago
|
||
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b64e32f1ceee Introduce a ResetLayerStateForRecycling() helper function. r=mstange https://hg.mozilla.org/integration/autoland/rev/2fb6c8087961 Call ResetLayerStateForRecycling() when recycling an OwnLayer. r=mstange https://hg.mozilla.org/integration/autoland/rev/f4b95c6cfe5f Call ResetLayerStateForRecycling() when recycling an image or color layer. r=mstange
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b64e32f1ceee https://hg.mozilla.org/mozilla-central/rev/2fb6c8087961 https://hg.mozilla.org/mozilla-central/rev/f4b95c6cfe5f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 17•7 years ago
|
||
Comment on attachment 8769345 [details] Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function. Let's uplift this to avoid shipping 49 with this regression. This uplift request applies to all three patches in this bug. Approval Request Comment [Feature/regressing bug #]: Bug 1267438 [User impact if declined]: On pages with certain structures, during scrolling we keep adding a new layer to the layer tree on every paint instead of reusing an existing layer. This can result in excessive memory usage and rendering problems. [Describe test coverage new/current, TreeHerder]: Tested manually, and patch has been on m-c since 206-07-26. [Risks and why]: Low risk. This is a small and well-understood change. [String/UUID change made/needed]: None.
Attachment #8769345 -
Flags: approval-mozilla-aurora?
Comment 18•7 years ago
|
||
Comment on attachment 8769345 [details] Bug 1285619 - Introduce a ResetLayerStateForRecycling() helper function. This patch fixes a regression. Take it in aurora.
Attachment #8769345 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/bd518331a09d https://hg.mozilla.org/releases/mozilla-aurora/rev/76fafe444a37 https://hg.mozilla.org/releases/mozilla-aurora/rev/c5fdc568bd3b
You need to log in
before you can comment on or make changes to this bug.
Description
•