1580178 - Hit test without a synchronous message to the render backend thread

Status: RESOLVED FIXED

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

Description

[Nicolas Silva [:nical]]

This will help a lot with bug 1542056. At the moment if the render backend thread is busy it takes a while for it to answer hit testing queries and the main thread ends up waiting for 10+ms in bad cases. Aggravating the input event buildup.

I think that we don't need hit testing queries to be serialized with transactions since any number of transactions can be inserted between the time the main thread gets the result and when it submits a transaction that depends on it.
That said I don't know the ins and outs of input handling and APZ enough to be sure that removing ordering constraints between hit test queries and previous webrender transactions is not going to break everything.

Comment 1

[Botond Ballo [:botond]]

My recollection is that when Kats implemented APZ hit testing with WR, the synchronous message to the render backend thread was an unfortunate but necessary part of the design. However, I don't remember the details.

Comment 2

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

The fundamental requirement we have with hit-testing is that the result that WR returns "makes sense" to APZ. Which in practice means that the scene against which WR does the hit-test was created from the same layer transaction that APZ last got in UpdateHitTestingTree. We currently ensure that the WR scene swap and APZ update happen in lockstep via the lock/unlock that happens on the updater thread around the scene swap, and this mutex prevents hit-testing from getting run concurrently.

From a quick glance at Nical's patch on the other bug, it seems like he's replacing the sync IPC with a mutex/shared data structure, which seems like it should still be compatible with the above mechanism. So from a high-level architecture point of view I don't think there's anything fundamentally wrong with Nical's approach. (Take with grain of salt, though, my memory of all this is a bit fuzzy and I may have forgotten additional sources of complexity.)

It would be worth doing a try push of all the APZ mochitests with the patch applied and mashing the retrigger button to see if any new intermittents crop up. If the "fundamental requirement" I stated above is violated, it usually manifests as one of these assertions failing, so doing many retriggers of the tests on debug builds across all platforms should catch any problems.

Comment 3

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) (away until Feb-2020) from comment #2)

If the "fundamental requirement" I stated above is violated, it usually manifests as one of these assertions failing,

Those assertions already fail sometimes :/ Just happened to me while testing an unrelated bug in debug mode. I believe Alexis has run into it as well.

Comment 4

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) (away until Feb-2020) from comment #2)

The fundamental requirement we have with hit-testing is that the result that WR returns "makes sense" to APZ. Which in practice means that the scene against which WR does the hit-test was created from the same layer transaction that APZ last got in UpdateHitTestingTree.

What about the frame against which WR does the hit-test? It looks like we recreate the hit tester on every frame, is it important that the frame used be synchronized with APZ's current view of async transforms and such?

Comment 5

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

Hmm, good question. I don't think any particular synchronization there is needed but I'm not totally sure.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) (away until Feb-2020) from comment #2)

The fundamental requirement we have with hit-testing is that the result that WR returns "makes sense" to APZ. Which in practice means that the scene against which WR does the hit-test was created from the same layer transaction that APZ last got in UpdateHitTestingTree. We currently ensure that the WR scene swap and APZ update happen in lockstep via the lock/unlock that happens on the updater thread around the scene swap, and this mutex prevents hit-testing from getting run concurrently.

From a quick glance at Nical's patch on the other bug, it seems like he's replacing the sync IPC with a mutex/shared data structure, which seems like it should still be compatible with the above mechanism. So from a high-level architecture point of view I don't think there's anything fundamentally wrong with Nical's approach. (Take with grain of salt, though, my memory of all this is a bit fuzzy and I may have forgotten additional sources of complexity.)

FWIW, I think Nical's patch does not work as written, as explained here. However, the problem described there may be solvable while keeping the same general approach.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Bug 1580178 - Allow hit-testing without synchronous messaging. r=botond,kats

This patch adds an asynchronous hit tester that can perform hit testing queries without blocking on a synchronous message to the render backend thread, which is often busy building frames. This is done by having a shared immutable hit tester readable by any thread, atomically swapped each time the render backend processes a new scene or frame.
In order to asynchronously hit test without causing race conditions with APZ intenral state, the hit tester has to be built while the APZ lock is held.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed2d1ba57e2f
Allow hit-testing without synchronous messaging. r=botond,kats

Comment 9

[Oana Pop-Rus]

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