1339625 - All the WR tests are orange on m-c

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

Something in [1] caused all the QR tests to go orange on m-c. See [2] for the before/after. We should hold off on doing m-c -> graphics merges until we figure this out.

[1] https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=1060668405a9399774c205430de4a7001d3f27ac
[2] https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=quantum&fromchange=c5a25b056d1e66fcaf67d680abea5a80d5944092&tochange=1060668405a9399774c205430de4a7001d3f27ac

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Luckily we should be able to bisect this on inbound by using the magical "Add jobs" and adding some QR jobs to inbound pushes. I'm doing that now.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Hm, the add jobs might not be working :(

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Try push on mattwoodrow's push shows the same orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a593780730b22d733deab2bdbcfcda6e688756e

So that narrows it down to https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e244431cfea1&tochange=a19ac5da6082

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I kicked off another try push using cset bcda57aa0c3b just before matt's patches to confirm [1]. But it's almost certainly a result of Matt's patches. Matt, do you have time to investigate? To build locally you need to add `ac_add_options --enable-webrender` to your mozconfig. It should run with webrender enabled by default, and you can flip it off using gfx.webrender.enabled=false (startup pref).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=07325befcd427474f01b4e0ae5e8e3294cb18842

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backtrace

I can repro locally on Linux. Seems to be some sort of deadlock, I think. I have to head out but here's a stacktrace where the code is stuck.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: unlock-textures-before-calling-updated

WebRenderPaintedLayer was calling Updated on the content client while it was still between Begin/EndPaint, so the textures were already locked when we tried to take the lock on behalf of the compositor.

ClientPaintedLayer had the same issue (fixed in [1]), so I guess WebRenderPaintedLayer inherited the bug when it was copied over.


[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/8255c2bb705a

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8837376 [details] [diff] [review]
unlock-textures-before-calling-updated

Review of attachment 8837376 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to fix it for me locally. Painting seems *super* slow though, not sure if that's from your patches or something else.

::: gfx/layers/wr/WebRenderPaintedLayer.h
@@ +51,5 @@
>    }
>  
>    Layer* GetLayer() override { return this; }
>    void RenderLayer() override;
> +  void PaintThebes(nsTArray<ReadbackProcessor::Update>* aReadbackUpdates)

needs trailing semicolon

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: dont-lose-end-frame

Compositor::EndFrame is where we release locks for textures, overriding it and not calling the base class impl probably means we never get to re-use textures (and will waste 500ms trying).

Untested, but I think this should fix it.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8837383 [details] [diff] [review]
dont-lose-end-frame

Review of attachment 8837383 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you think this is right, but I applied it locally and it didn't seem to fix the slowness.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

The first patch by itself also didn't fix all the failures. I assume that on try it's also running slow and getting killed due to timeout.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85ed34a3a079894fd625e25937d139e37e64b3f6

Comment 12

[Kartikaya Gupta (email:[e-mail])]

I started another try push with both patches applied, just in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=182aee71381a41963886f21aafbec46ef9b78fae

Comment 13

[Matt Woodrow (:mattwoodrow)]

I guess we're not calling EndFrame on our fake Compositor instance?

We probably need to do that after WR has finished compositing, but I don't really know where the right place is.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I don't think we are even using that class anymore. Bug 1339323 is removing it.

Comment 15

[Sotaro Ikeda [:sotaro]]

I also reproduce the problem. I am going to look into read lock problem.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Enable double buffering on LAYERS_WR

With attachment 8837376 [details] [diff] [review], the problem was addressed for me.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8837439 [details] [diff] [review]
patch - Enable double buffering on LAYERS_WR

Review of attachment 8837439 [details] [diff] [review]:
-----------------------------------------------------------------

Can you debug why it isn't working with single buffering?

We don't have a way to render to textures directly with WR afaik, so we must be uploading to another texture. That would give us 3 (or 4?) buffers if we're using ContentClientDoubleBuffered, which isn't ideal.

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> 
> Can you debug why it isn't working with single buffering?

If webrender is enabled, rendering to webrender is not optimised. CompositableHost::GetAsSurface() is called to get rendering surface. Composite() is not called and Compositor is not used.
  > https://dxr.mozilla.org/mozilla-central/source/gfx/layers/wr/WebRenderBridgeParent.cpp#293

When LayerManagerComposite with hw acceleration is used, BufferTextureHost::MaybeUpload() calls ReadUnlock() and unlock the TextureHost. MaybeUpload() is triggered by AutoLockCompositableHost. But that code path is not used if webrender is enabled. It needs Compositor, but Compositor is not used if webrender is enabled. 

There is another route to call ReadUnlock(). TextureHost::ReleaseCompositableRef() also triggers ReadUnlock(). attachment 8837439 [details] [diff] [review] uses it. Second buffer drops ref of first buffer in ContentHost.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Force clear Texture

Comment 20

[Sotaro Ikeda [:sotaro]]

attachment 8837486 [details] [diff] [review] also address the problem. It also uses TextureHost::ReleaseCompositableRef() to trigger ReadUnlock().

Comment 21

[Sotaro Ikeda [:sotaro]]

attachment 8837486 [details] [diff] [review] caused crash on local debug build. But failed to attach gdb...

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8837486 [details] [diff] [review]
patch - Force clear Texture

Review of attachment 8837486 [details] [diff] [review]:
-----------------------------------------------------------------

So, I think what happens is:

Our TextureHosts have the UPLOAD_IMMEDIATE flag, so we try upload them to a texture during the transaction. We don't have a compositor though, so MaybeUpload() fails (does it print a warning?), and doesn't hit the ReadUnlock path.

Normally, once we've uploaded the changes into the appropriate TextureSource, we know we won't read from the TextureHost again, so it's safe to ReadUnlock the contents.

I assume the same probably holds true here, once we've processed TOpDPPushExternalImageId, we shouldn't read from the textures within the CompositableHost again. We're sort out bypassing the design of TextureHost/TextureSource/CompositableHost though.

Given that, I think this (or just calling ReadUnlock directly) should be safe, but it's a bit scary.

Lets see what nical thinks.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Can't we just use a fake Compositor (or pull the functionality out into a shared abstract class TextureSourceProvider?) to allocate TextureSources and do the upload using the normal gecko way?

Then we can just give WR the texture ID, rather than the memory pointer?

Comment 24

[Intermittent Failures Robot]

386 failures in 143 pushes (2.699 failures/push) were associated with this bug yesterday.  
Repository breakdown:
* mozilla-central: 386

Platform breakdown:
* linux64-qr: 383
* linux64-stylo: 2
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339625&startday=2017-02-15&endday=2017-02-15&tree=all

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Force clear Texture

Fixed crash on debug build.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8837383 [details] [diff] [review]
dont-lose-end-frame

Obsoleting this patch as it doesn't help and the file has been removed in the graphics branch.

With the two other patches (Matt's first patch and Sotaro's follow-up) things seem fixed to me. I'd like to get these landed ASAP as this bug is blocking doing merges between graphics and m-c.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=807844c2cf6557e94eb86c1f26724971fb4ac96f

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Try push is green.

Comment 29

[Intermittent Failures Robot]

560 failures in 151 pushes (3.709 failures/push) were associated with this bug yesterday.  
Repository breakdown:
* mozilla-central: 560

Platform breakdown:
* linux64-qr: 560

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339625&startday=2017-02-16&endday=2017-02-16&tree=all

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Force clear Texture

Cleaner patch, it uses WebRenderImageHost for WebRenderPaintedLayer, instead of ContentHostSingleBuffered. We use ContentHostSingleBuffered same way as WebRenderImageHost when wr is enabled.

WebRenderImageHost is wr specific CompositableHost implementation. It is cleaner to add wr specific workaround.

Comment 31

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06c5fde05a3394950d530faa8414edbd92d30e87

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5761d8f5d23
Unlock textures before calling updated r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/264c1c5ac2c5
Force clear Texture r=kats

Comment 33

[Kartikaya Gupta (email:[e-mail])]

(In reply to Pulsebot from comment #32)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/264c1c5ac2c5
> Force clear Texture r=kats

This should be r=nical

Comment 34

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

https://hg.mozilla.org/mozilla-central/rev/c5761d8f5d23
https://hg.mozilla.org/mozilla-central/rev/264c1c5ac2c5

Comment 35

[Intermittent Failures Robot]

481 failures in 180 pushes (2.672 failures/push) were associated with this bug yesterday.  
Repository breakdown:
* mozilla-central: 481

Platform breakdown:
* linux64-qr: 480
* android-4-0-armv7-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339625&startday=2017-02-17&endday=2017-02-17&tree=all

Comment 36

[Intermittent Failures Robot]

1525 failures in 833 pushes (1.831 failures/push) were associated with this bug in the last 7 days. 

This is the #1 most frequent failure this week. 

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **

Repository breakdown:
* mozilla-central: 1525

Platform breakdown:
* linux64-qr: 1521
* linux64-stylo: 2
* linux32: 1
* android-4-0-armv7-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339625&startday=2017-02-13&endday=2017-02-19&tree=all

Comment 37

[Sotaro Ikeda [:sotaro]]

(In reply to Wes Kocher (:KWierso) from comment #34)
> https://hg.mozilla.org/mozilla-central/rev/c5761d8f5d23
> https://hg.mozilla.org/mozilla-central/rev/264c1c5ac2c5

It seems to break chrome rendering on graphic branch but not on m-c.