Open Bug 1116579 Opened 9 years ago Updated 6 months ago

Add a direct-dispatch code path for touch events destined to a child process

Categories

(Core :: Panning and Zooming, defect, P3)

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

People

(Reporter: kats, Unassigned)

References

Details

(Keywords: perf, Whiteboard: gfx-noted)

Attachments

(3 files, 8 obsolete files)

Sample tree
3.53 KB, text/plain
Details
Part 1 - Ensure that APZ doesn't bypass dispatching to process with touch listeners
4.03 KB, patch
Details | Diff | Splinter Review
Part 2 - Direct-dispatch events to child processes when possible
5.95 KB, patch
Details | Diff | Splinter Review 
In bug 920036 touch inputs get rerouted so that they go through the APZ first, before going through the gecko event-dispatching code. After going through the APZ, we can sometimes tell that the touch input doesn't need to go through the root process at all, and that we can send it directly to a child process. This is basically an optional optimization, but it will help reduce latency when delivering input events to the child process in many cases.

This bug tracks this work.
Attached patch WIP (obsolete) — — Splinter Review 

    
    

    
  
This can probably be landed too now as an optimization. I'll need to test it and stuff.
Assignee: nobody → bugmail.mozilla

    
    

    
  
Is this an optimization for the cases where TryCapture returns false?

    
    

    
  
No, it's an optimization for the case where we know that (a) the event is going to go to the child process and (b) nothing in the parent process cares about the event. In this case we don't need to send the event through the root process event dispatch.

    
    

    
  
I suspect (but have not confirmed) that at the moment this patch won't have much of an effect because of the entire-screen touch listeners in the gaia system process (bug 1130011, bug 1130016). These listeners will force us to send the touch event through the parent process, so until those are all removed there's not much point in landing this. In fact landing this before those are early might just make it harder to trace regressions back to this bug.
Depends on: 1130011, 1130016

    
    

    
  
Actually as it turns out we never fixed the issue described at https://bugzilla.mozilla.org/show_bug.cgi?id=1125422#c7 and so even if there is a touch listener in the parent process, the patch on this bug will (erroneously) direct-dispatch the touch event to the child process. So we'd probably have to land attachment 8554702 [details] [diff] [review] and attachment 8554703 [details] [diff] [review] (or some variant thereof) before we can land the patch on this bug.

    
    

    
  


  
This is attachment 8554703 [details] [diff] [review] resurrected and rebased. However it doesn't have the desired effect in my local testing. Will investigate more at some point.

        Attachment #8566516 -
        Flags: review-

    
    

    
  


  
My original WIP rebased.

        Attachment #8542687 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Actually as it turns out we never fixed the issue described at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1125422#c7 and so even if there
> is a touch listener in the parent process, the patch on this bug will
> (erroneously) direct-dispatch the touch event to the child process. So we'd
> probably have to land attachment 8554702 [details] [diff] [review] and
> attachment 8554703 [details] [diff] [review] (or some variant thereof)
> before we can land the patch on this bug.

I thought those patches were superseded by the ForceDispatchToContent flag.

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #10)
> I thought those patches were superseded by the ForceDispatchToContent flag.

I thought so too but it's not quite the case. The ForceDTC flag will annotate the tree so that listeners at the window-level (which were previously getting ignored) are now "known". However the hit-testing in the APZ will still find the deepest target first, even if that is in the child process and the listener is in the parent process. The way the code is in master that's fine, because we always dispatch the touch events into the parent process anyway but the purpose of this bug was to bypass that step when possible.

    
    

    
  

      
      
      
      

        Attached file
          
        Sample tree
        — 
      

    


  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> This is attachment 8554703 [details] [diff] [review] resurrected and
> rebased. However it doesn't have the desired effect in my local testing.
> Will investigate more at some point.

The reason it doesn't work is that the tree looks like the attached. Specifically node 0xaf4b7340 which should be the one that matches doesn't actually have a hit region, and so the HitTest call on it returns HitNothing instead of HitDispatchToContentRegion and we keep going deeper in the tree (and hit for example 0xaf81b6c0).

If we check for that flag before checking the hitregion inside HitTest, then we'll end up matching things like 0xaf4b71c0, which is also wrong. So I think we need to recurse all the way down until we hit something, and then walk up the chain from there and use the rootmost ancestor that has a force-dtc flag or encompassing dtc region.

    
    

    
  


  
Ugh. All my patches were off-by-one. Reuploading.

        Attachment #8576860 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #8576862 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #3)
> Is this an optimization for the cases where TryCapture returns false?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> No, it's an optimization for the case where we know that (a) the event is
> going to go to the child process and (b) nothing in the parent process cares
> about the event. In this case we don't need to send the event through the
> root process event dispatch.

This response finally makes sense to me now: the idea here is to *replace* TryCapture, not to add an additional optimization on top of it.

    
    

    
  
Yeah, sorry if that wasn't clear before. The advantage here is that (a) even the touch-start won't have to go through the root-process main-thread dispatch (which happens with the TryCapture method) and (b) less icky state management. Also maybe (c) dispatching the event directly from the compositor thread to the child process, without having to touch the root-process main thread at all. Although I'm not sure if the IPC code allows that.

    
    

    
  
gtests pass on these patch but try is closed and I want to get a green try run before requesting review as there's significant potential to break stuff in testing.

    
    

    
  
So I'm having second thoughts about the "part 1" patch here. One concern is that now we might end up with a tentative APZC target that's not so good, and if we hit the listener timeout then we'll end up scrolling that instead of one that we know is better. For example consider a tree like this all in the same process:

ContainerLayer A with scrollable metrics A and force-dispatch-to-content
  ContainerLayer B
    PaintedLayer C with scrollable metrics B

if the hit-test finds C we'll nonetheless use A as the tentative target, and will scroll A if the listeners take too long. I think it makes more sense to keep scrolling C (which is our current behaviour).

The problem with the current behavior is when C is in a child process relative to A. In that case the part 2 patch will skip over the listeners in the parent process and directly dispatch the event to the child process. However I notice that even with the current code, the TryCapture code path does something similar - once the capture path is invoked the events are shunted directly to the child process and the parent process doesn't get them. In fact if I register touchstart/move/end listeners on the B2G parent process window, and I scroll the homescreen, only the touchstart/end are picked up by my listeners (because all the move events get captured and never go through the root process dispatch). This is clearly wrong - either all of the events should get picked up or none of them should.

If we ditch "part 1" then none of the events will get picked up, and if we keep the "part 1" then all of them will get picked up (with the problem that I described above of possibly scrolling the wrong APZC).

    
    

    
  
I convinced myself that we do need the events to get picked up. My rationale was that if we compare the desktop e10s vs non-e10s case, they should both behave identically in terms of listeners that get triggered by events. If we do a direct-dispatch to child process, then that will not be the case, as the parent process will not get triggered. Although the desktop e10s case is subtly different from the b2g case, they are equivalent from the APZ point of view and so they should be treated the same.

The only way I can think of to implement this without sacrificing the "tentative APZ" behaviour is to pass out an additional out-param or return value from APZCTreeManager::ReceiveInputEvent to indicate if there is a listener in the parent process. But I'm not really a fan of this, so maybe I'll hold off doing this for now. I'd still like to remove the patch to get rid of the TryCapture code path as that leads to incorrect behaviour and is also not really needed for perf reasons any more. I'll split that out into another bug.
No longer blocks: 1012945

    
    

    
  
Comment on attachment 8576867 [details] [diff] [review]
Part 3 - TryCapture, meet /dev/null

Moved this patch to bug 1143518.

        Attachment #8576867 -
        Attachment is obsolete: true
Assignee: bugmail.mozilla → nobody
Whiteboard: gfx-noted

    
    

    
  
The patches here have rotted but I think the goal stated in comment 0 is probably still valid.
OS: Gonk (Firefox OS) → All

    
    

    
  
Note that since this bug was filed we've added support for passive listeners, which don't affect the fdtc flag. So we would have to add a new flag to indicate the presence of touch listeners in the parent process if we wanted to still do this.
Priority: -- → P3

    
  
Severity: normal → S3

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: