1148131 - Reduce time spent validating tiles

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

We (sometimes, at least) spend almost half our paint time in ClientTiledLayerBuffer::ValidateTile(). That seems like a long time to me.

Comment 1

[Nicolas Silva [:nical]]

(In reply to James Willcox (:snorp) ([e-mail]) 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?

Comment 2

[Honza Bambas (:mayhemer)]

The main question is - does it need to happen on the main thread?

Comment 3

[Nicolas Silva [:nical]]

(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.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Profile

Actually this one does kind of show the problem. I'm looking at 2741 - 2977, and 30% of the time is in ValidateTile.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

20% of the time is under the PaintThebesSingleBufferAlloc label, which just creates a DrawTarget. Geez.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Filed bug 1148526 for the issue in comment #6

Comment 8

[Nicolas Silva [:nical]]

(In reply to James Willcox (:snorp) ([e-mail]) 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).

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: 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?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

pixman_composite_src_8888_8888_asm_neon is basically memcpy. This is probably copying the tile contents.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

This is showing up in BHR data to. http://telemetry.mozilla.org/hang/bhr/

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Nical can you link to the patches that switch to draw target tiled so that we can see what difference that makes?

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 15

[Nicolas Silva [:nical]]

Attachment: DrawTargetTiled on android

Comment 16

[Lee Salzman [:lsalzman]]

What site were you browsing while testing this issue, to make this more likely to reproduce?

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

(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

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Nical, that patch seems to work pretty well for me. How much work to land it, maybe behind a pref?

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 20

[Nicolas Silva [:nical]]

(In reply to James Willcox (:snorp) ([e-mail]) 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).

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Enable DrawTargetTiled on Android

I needed to fuzz one test that already had some fuzzing, and disable another
due to some svg weirdness (filed as bug 1201937)

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69e7d484b749

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

I had to back this out due to 4.0 debug reftest failures

https://hg.mozilla.org/integration/mozilla-inbound/rev/2883757f7941

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aea6d833cad5

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aea6d833cad5