Closed Bug 624454 Opened 13 years ago Closed 13 years ago

Tune displayport size

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(fennec2.0+)

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: cjones, Assigned: wesj)

References

Details

(Whiteboard: [part 1 landed - more coming?])

Attachments

(2 files, 2 obsolete files)

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.
Oops, there's also

 (3) Smaller displayport requires less memory
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Assignee: nobody → jones.chris.g
This is a fennec frontend project, out of my wheelhouse.
Assignee: jones.chris.g → nobody
Assignee: nobody → ben
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
Assignee: ben → wjohnston
is this just a meta bug now?
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).
(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
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #516307 - Flags: review?(mark.finkle)
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
(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.)
Depends on: 634397
Attached patch Patch (obsolete) — Splinter Review
Oops. Wrong patch.
Attachment #516307 - Attachment is obsolete: true
Attachment #516426 - Flags: review?(21)
Attachment #516307 - Flags: review?(mark.finkle)
Attached patch Patch v1Splinter Review
Argh. nope. wrong bug.
Attachment #516426 - Attachment is obsolete: true
Attachment #516428 - Flags: review?(mark.finkle)
Attachment #516426 - Flags: review?(21)
Comment on attachment 516428 [details] [diff] [review]
Patch v1

Let's land this and get feedback on the change
Attachment #516428 - Flags: review?(mark.finkle) → review+
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.
Status: NEW → ASSIGNED
Whiteboard: [needs-landing]
Actually height increase definitely needed + repainting visible area at first.
Whiteboard: [needs-landing] → [part 1 landed - more coming?]
what's the plan here? Can this be closed? Perhaps follow up work should be done in another bug.
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.
Attached patch Patch 2Splinter Review
This feels a better to me.
Attachment #518441 - Flags: review?(ben)
Comment on attachment 518441 [details] [diff] [review]
Patch 2

That's what I've been playing with too.
Attachment #518441 - Flags: review?(ben) → review+
pushed: http://hg.mozilla.org/mobile-browser/rev/34ca5bea3ebe

closing this bug now.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 640444
Is there a way to test this on a user-facing side?
Depends on: 644665
You need to log in before you can comment on or make changes to this bug.