Discovered using vlad's awesome new tool! (bug 830881) STR (1) Load nytimes.com with layerscope running (2) Pan up and down homepage We continually switch between buffer sizes from height=568 to height=1038 in small increments. I recall that there's an element of this that's based on computed pan velocity. It looks like we're copying content into the new buffers properly, but this kind of churn will cause excessive memory and ipc traffic. We need to see why - thebes layer keeps throwing out buffers that are big enough for its uses - or failing that, improve the apzc re-request heuristics

This also reproduces with cnn.com, and it's a bit friendlier to work with.

This appears to have been done deliberately in order to increase the buffer size during skating. However I feel that making the buffer bigger during skate is counterproductive because it ends up with more to paint.

The displayPort also gets intersected with mScrollableRect so it gets smaller as you approach the edges of the scrollable area.

This patch is available for test but not ready for review.

Do you have a feel for any perf differences?

(Went to test this but it's quite rotted on gecko-18.)

Getting tiling working on b2g is a good way to resolve this issue.

Important content issue that crops up all the time with browser content.

approaches the edges of the scrollable rect.

Comment on attachment 754654 [details] [diff] [review] Reduce APZC buffer churn Review of attachment 754654 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/BaseRect.h @@ +419,5 @@ > } > > + /** > + * Clamp aRect to this rectangle. It is allowed to end up on any > + * edge of the rectangle. This is a poor definition of what clamping to a rectangle is. And the implementation doesn't match what I intuitively think clamping to a rectangle should be. For example I expect (x=5,y=5,width=10,height=2).clamp((x=0,y=0,w=10,h=10) => (x=5,y=5,w=5,h=2). I don't understand 'It is allowed to end up on any edge of the rectangle' on this context either. It would be great to add some unit tests for this function as well. You can add these tests to the existing Azure unit tests.

(In reply to Benoit Girard (:BenWa) from comment #11) > (x=5,y=5,width=10,height=2).clamp((x=0,y=0,w=10,h=10) => (x=5,y=5,w=5,h=2). That would be intersect. The purpose here is to move the co-ordinates without adjusting the size. I couldn't think of a better name. Perhaps ForcedIntoBoundsOf() would be more descriptive. > I don't understand 'It is allowed to end up on any edge of the rectangle' on > this context either. Neither do I. It must've made sense to me at the time I wrote it :-P > It would be great to add some unit tests for this function as well. You can > add these tests to the existing Azure unit tests. Sounds like a good plan.

approaches the edges of the scrollable rect.

https://tbpl.mozilla.org/?tree=Try&rev=93b7551786e2 https://tbpl.mozilla.org/?tree=Try&rev=6c9b263df7b9

Comment on attachment 755687 [details] [diff] [review] Reduce APZC buffer churn [Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: Less responsive around the edges of the scrollable region. Testing completed: scrolling in browser and printing the viewport sizes Risk to taking this patch (and alternatives if risky): Incorrect viewport calculations could cause it to paint the wrong thing. Seems unlikely. String or UUID changes made by this patch: none

This would typically fall outside of 1.1 scope at this point since it's not a new regression and isn't partner required. As the B2G perf owner, what do you think mlee?

With this patch the viewport is 2.5 x 3.5 screens in size (more during fling). When at a corner it will reduce linearly to 1.75 x 2.25 screens as it approaches. The fix eliminates the viewport shrinking and therefore the buffer churn. The bigger (or at least constant size) buffers reduce the repainting when coming away from the edges.

I'd pull this into 1.1. The impact of not fixing this is "excessive memory and ipc traffic" per Chris Jones. The risk called out in the Approval Request should be addressed by the the Unit Tests pending review. Ben Wa, can we get your r+ on the Unit Tests patch?

Comment on attachment 755688 [details] [diff] [review] Unit tests for moz2d repo Review of attachment 755688 [details] [diff] [review]: ----------------------------------------------------------------- Cool! Nice work ::: unittest/TestRect.h @@ +1,4 @@ > +/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*- > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ Testing code can be licensed public domain. @@ +2,5 @@ > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#pragma once Don't use pragma once, see dev.platform discussion. Our build system doesn't handle it properly.

(In reply to Mike Lee [:mlee] from comment #21) > The risk called out in the Approval Request should be addressed by the the > Unit Tests pending review. The unittest only cover ClampRect, not the displayport changes themselves. > > Ben Wa, can we get your r+ on the Unit Tests patch? done

(In reply to Benoit Girard (:BenWa) from comment #23) > > The unittest only cover ClampRect, not the displayport changes themselves. > Thanks Ben, does that mean the risk that "Incorrect viewport calculations could cause it to paint the wrong thing" still exists? Anthony, Can we add tests to cover the displayport changes?

(In reply to Mike Lee [:mlee] from comment #24) > (In reply to Benoit Girard (:BenWa) from comment #23) > > > > The unittest only cover ClampRect, not the displayport changes themselves. > > > > Thanks Ben, does that mean the risk that "Incorrect viewport calculations > could cause it to paint the wrong thing" still exists? Ths unit tests cover ClampRect. Aside from that we have a one line change in APZC. There is a very small possibility of something going wrong. If something does somehow go wrong then it will show up as unpainted regions. It would be very hard for there to be a bug and for to not be obvious. I've alreay said that it seems unlikely. > Can we add tests to cover the displayport changes? I could write some tests based on the work in bug 864441 to test CalculatePendingDisplayPort. However this bug isn't a priority for me right now so feel free to approval-

The moz2d tests currently only run out of the moz2d repo and not on m-c, b2g, etc.

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #25) > ... > I could write some tests based on the work in bug 864441 to test > CalculatePendingDisplayPort. However this bug isn't a priority for me right > now so feel free to approval- Anthony, What's the memory and ipc traffic delta with and without your patch?

We paint 4 times per second. Without this patch we allocate a new buffer each time we paint near the edges. Near means within 1.25 screens vertically and 0.75 horizontally although these values are slightly different during fling. With the patch we don't paint at all when we're near both horizontal and vertical edges. We only allocate a new buffer when we start/end fling or when we show/hide the url bar.

Approving for 1.1 given mlee's comment 21.

Backed out on b2g18 for red on OSX. https://hg.mozilla.org/releases/mozilla-b2g18/rev/7772c18ffdec

Looks like branch-patch is no longer needed here, leaving it to Ryan to confirm and remove keyword.

Heh, I actually did notice that I'd forgotten to clear it after pushing to b2g18. But I decided to save a bugmail and just get it when I did the uplift to v1.1hd. So much for that idea :-) https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5aa800260700