Closed Bug 1148131 Opened 5 years ago Closed 4 years ago

Reduce time spent validating tiles

Categories

(Core :: Graphics: Layers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: snorp, Unassigned)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 1 obsolete file)

We (sometimes, at least) spend almost half our paint time in ClientTiledLayerBuffer::ValidateTile(). That seems like a long time to me.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #0)
> We (sometimes, at least) spend almost half our paint time in
> ClientTiledLayerBuffer::ValidateTile(). That seems like a long time to me.

Validate tile does the texture allocation and copying (of from the "single paint buffer" and also the copy-on-write mechanism betwen front and back buffer) and a bunch of region calculation. Do you have a profile handy with what in ValidateTile is taking that long?
The main question is - does it need to happen on the main thread?
(In reply to Honza Bambas (:mayhemer) from comment #2)
> The main question is - does it need to happen on the main thread?

It needs to happen on the painting thread which currently is the main thread. Off-main-thread painting is a cool project that we have been talking about for quite a while but requires so much work on the layout side (thread-safe display lists) that we've decided we still have lower hanging fruits to pick every time it has come up in the discussion so far.
Hmm, I did have a profile showing a lot of time here, but the latest one looks like it's doing alright. Maybe it was an anomaly. I'll dig some more.

What texture allocation do we need to do there for Android? Do you mean creating the TextureHost?
Attached file Profile (obsolete) —
Actually this one does kind of show the problem. I'm looking at 2741 - 2977, and 30% of the time is in ValidateTile.
20% of the time is under the PaintThebesSingleBufferAlloc label, which just creates a DrawTarget. Geez.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> What texture allocation do we need to do there for Android? Do you mean
> creating the TextureHost?

Yes (the TextureClient in fact).
Attached file Better profile
This profile has symbolication and shows that much of the time in ValidateTile() is spent in pixman_composite_src_8888_8888_asm_neon. I think maybe that's the FillRect calls towards end for component alpha?
Attachment #8584705 - Attachment is obsolete: true
pixman_composite_src_8888_8888_asm_neon is basically memcpy. This is probably copying the tile contents.
Nical can you link to the patches that switch to draw target tiled so that we can see what difference that makes?
Flags: needinfo?(nical.bugzilla)
Duplicate of this bug: 1148526
He put it in 1148526. I'll just dup that bug to this one since they're both the same class of problem with tiles it seems.
Flags: needinfo?(nical.bugzilla)
Whiteboard: [gfx-noted]
What site were you browsing while testing this issue, to make this more likely to reproduce?
Flags: needinfo?(snorp)
(In reply to Lee Salzman from comment #16)
> What site were you browsing while testing this issue, to make this more
> likely to reproduce?

This was with the nytimes site copy found in the ep1 repo used by eideticker, found here: http://git.mozilla.org/?p=automation/ep1.git;a=summary
Flags: needinfo?(snorp)
Nical, that patch seems to work pretty well for me. How much work to land it, maybe behind a pref?
Flags: needinfo?(nical.bugzilla)
The work is tracked in bug 1083101.  :snorp and I talked offline about possibility to land this for Fennec, focusing on the "real problems" rather than reftest problems.  Obviously, that's a fine line, but let's see if we can work out the details as to what that really means.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #18)
> Nical, that patch seems to work pretty well for me. How much work to land
> it, maybe behind a pref?

The important part of the patch is the pref. The rest triggers a very mysterious bug on the B2G ics emulator (thing that we won't ship but only thing that we test against), that I haven't managed to figure out so far so it has stalled for now.
If you enable the pref without the rest of the patch you'll probably get some sampling artefacts (although it might be just on b2g's overscroll effect since mac is doing fine). but most of the functionality is already preffed off in tree. I don't know how much work will be required to figure the remaining reftest failures out (+ the b2g ics issue).
Flags: needinfo?(nical.bugzilla)
I needed to fuzz one test that already had some fuzzing, and disable another
due to some svg weirdness (filed as bug 1201937)
Attachment #8657261 - Flags: review?(bas)
Attachment #8657261 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/aea6d833cad5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.