624454 - Tune displayport size

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There are two desiderata to be balanced

 (1) Larger displayport allows fast-panning longer in the chrome process before requesting re-rendering from content
 (2) Smaller displayport allows content to re-render more quickly

Tuning this size is really something we want to measure quantitatively over a benchmark suite.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oops, there's also

 (3) Smaller displayport requires less memory

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is a fennec frontend project, out of my wheelhouse.

Comment 3

[Oleg Romashin (:romaxa)]

I tried toolkit.browser.cacheRatioHeight", 4000, and it feels much better on simple page, at least single kinetic scroll in landscape mode does not show checkerboard.

time:		avail:	size:	rss:	clean:	shared:	private: change:
4000 height
content
20:14:06	203252	145096	47388	540	884	23988	   +0 kB
chrome
20:14:22	203280	156580	60100	11868	3080	20408	   +0 kB

2200 height
content:
20:11:53	197260	137784	44428	524	884	21040	   +0 kB
chrome:
20:11:38	197268	156828	61360	12012	3076	21620	   +0 kB

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

is this just a meta bug now?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Why do you think that?  Finding an optimal displayport for scrolling benchmark(s) while minimizing memory usage seems pretty well scoped.  What this project lacks is the scrolling benchmark(s).

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #5)
> is this just a meta bug now?

Nope. Getting new pref values, like Oleg tried in comment 3, is what this particular bug is about. Balancing memory use with decrease in checkerboarding.

We have metrics for memory, but objective metrics for panning / checkerboarding are lacking. We have been using subjective evaluation for checkerboarding.

We could add some timing code to see how much time is spent rendering the checkerboard background - I think it is only rendered as needed, so less checkerboarding means less time spent rendering it.

Until we have objective metrics, subjective will have to be good enough:
* How much vertical panning is checkerboard free?
* Zoom in and try vertical panning again
* How much horizontal panning is checkerboard free?
* Zoom in and try again

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch

Talked to stechz about this. He's done some pretty extensive panning in his days, and seems to think that just increasing the cache width will give us what we want.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 516307 [details] [diff] [review]
Patch

I'm fine with this change - horizontal checkerboarding , especially while zoomed, is bad right now. I do want to now if we can extend the vertical direction as well.

Changing the height to 3000 or 4000 would help squash more checkerboarding, and it looks like memory usage does not go up very much.

Tell me why we don't need (or want) to do the vertical too

Comment 11

[Matt Brubeck (:mbrubeck)]

(In reply to comment #10)
> Changing the height to 3000 or 4000 would help squash more checkerboarding, and
> it looks like memory usage does not go up very much.
> 
> Tell me why we don't need (or want) to do the vertical too

As comment 0 notes, the other disadvantage of a too-large displayport is that it takes longer to redraw.  You'll run into checkerboard less often, but it may stick around longer when you do.  So we should test this before making any height increase.  (And we should re-evaluate after bug 634397 lands, since that will change the results.)

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch

Oops. Wrong patch.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Argh. nope. wrong bug.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 516428 [details] [diff] [review]
Patch v1

Let's land this and get feedback on the change

Comment 15

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/mobile-browser/rev/0811c1cc3170

Wait to mark fixed? I am working on a different patch to help quantize checkerboard area shown/sec during a pan.

Comment 16

[Oleg Romashin (:romaxa)]

Actually height increase definitely needed + repainting visible area at first.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

what's the plan here? Can this be closed? Perhaps follow up work should be done in another bug.

Comment 18

[Wesley Johnston (:wesj)]

Given we are expecting big changes in checkerboarding, we will likely need to revisit this later.

Since we're just playing with prefs, I think the plan was to hold off on closing this until after that.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch 2

This feels a better to me.

Comment 20

[Benjamin Stover (:stechz)]

Comment on attachment 518441 [details] [diff] [review]
Patch 2

That's what I've been playing with too.

Comment 21

[Wesley Johnston (:wesj)]

pushed: http://hg.mozilla.org/mobile-browser/rev/34ca5bea3ebe

closing this bug now.

Comment 22

[Aakash Desai [:aakashd]]

Is there a way to test this on a user-facing side?