Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Description • 8 years ago

Right now if mouse drag events and touch events are interleaved, the code in InputQueue.cpp can't really manage them well, and it keeps clobbering and creating new input blocks. We need to be able to handle this scenario better, in particular before releasing touch on Windows. Also because the touch simulation in devtools exercises this scenario, where it synthesizes native touch events based on mouse input and so they can get interleaved very easily.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 8 years ago

Fixing the interleaving between drag and wheel should also take care of bug 1269067.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 2 • 8 years ago

Currently the input queue keeps a list of input blocks, and each input block has a list of input events. This setup makes it hard to handle interleaved input types properly - if you get a touch, mouse, touch, the two touch inputs should really go into the same block but right now they might get split up.

What I would like to do is change it so that the InputQueue just keeps a list of input events, and each input event can then have a pointer to a block state object, which is shared across all events in the same block.

We do somthing similar to this with the DragTracker object hanging off the InputQueue but we'll have to change it up a bit.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 3 • 8 years ago

WIP so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=752836bbf5d5

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 8 years ago

Updated: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a757db0ab2d5

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 8 years ago

I'll put my patches up for review tomorrow, after bug 1302093 and bug 1302128 are in central and I can rebase and push to MozReview more sanely. The try run in comment 4 is green; I didn't make any substantive changes since that try push. I did verify that the patches fix bug 1269067, and I wrote a gtest for that bug as well (which I'll post to that bug).

The patches I have are structured like so:

- The first few patches add the new data structure that will be used in InputQueue to track the input events. This is similar to the mInputBlockQueue that is currently there, but instead of being a list of blocks, each of which holds a list of events, the new mQueuedInputs just holds a list of "QueuedInput" objects. Each QueuedInput object is an event along with a pointer to the associated block. Effectively, InputQueue changes from holding a list of blocks to a list of events.

- Once the new data structure is in place (parallel to the existing data structure), I update various pieces of code to use the new data structure instead of the old one.

- Finally, once all the stuff is migrated over, I remove the old data structures and do some misc cleanup.

Note that there are some semantic differences between how the old structures worked and the new ones worked. In particular, in the new code, there isn't just one active block, but potentially one active block of each input type. Fundamentally, this is the main change that allows us to interleave input events and still associate the new events with the right input blocks. However it also changes the meaning of CurrentBlock() and friends, so I renamed those and updated the call sites accordingly in one of the patches.

	Botond Ballo [:botond]
	Comment 18 • 8 years ago
mozreview-review

Comment on attachment 8790522 [details]
Bug 1289432 - Use RefPtr instead of UniquePtr to hold the block state objects, since there will be multiple references to them soon.

https://reviewboard.mozilla.org/r/78294/#review76982

	Botond Ballo [:botond]
	Comment 19 • 8 years ago
mozreview-review

Comment on attachment 8790523 [details]
Bug 1289432 - Have the InputQueue keep RefPtrs to the active block of each input type.

https://reviewboard.mozilla.org/r/78296/#review76986

	Botond Ballo [:botond]
	Comment 20 • 8 years ago
mozreview-review

Comment on attachment 8790524 [details]
Bug 1289432 - Introduce a skeleton QueuedInput class.

https://reviewboard.mozilla.org/r/78298/#review77002

Please add to the commit description: "Note that this patch makes the InputData hierarchy polymorphic".

	Botond Ballo [:botond]
	Comment 21 • 8 years ago
mozreview-review

Comment on attachment 8790525 [details]
Bug 1289432 - Start populating the mQueuedInput array with queued input objects.

https://reviewboard.mozilla.org/r/78300/#review77014

::: gfx/layers/apz/src/InputQueue.cpp:178
(Diff revision 1)
>      INPQ_LOG("dropping event due to block %p being in mini-slop\n", block);
>      result = nsEventStatus_eConsumeNoDefault;
>    }
>    if (!MaybeHandleCurrentBlock(block, aEvent)) {
>      block->AddEvent(aEvent.AsMultiTouchInput());
> +    mQueuedInputs.AppendElement(MakeUnique<QueuedInput>(aEvent.AsMultiTouchInput(), *block));

|aEvent| already has type |MultiTouchInput|, so there is no need to call |AsMultiTouchInput()| on it.

(The same goes for the previous line, which was pre-existing.)

::: gfx/layers/apz/src/InputQueue.cpp:238
(Diff revision 1)
>      *aOutInputBlockId = block->GetBlockId();
>    }
>  
>    if (!MaybeHandleCurrentBlock(block, aEvent)) {
>      block->AddEvent(aEvent.AsMouseInput());
> +    mQueuedInputs.AppendElement(MakeUnique<QueuedInput>(aEvent.AsMouseInput(), *block));

Likewise here.

::: gfx/layers/apz/src/InputQueue.cpp:291
(Diff revision 1)
>    if (aOutInputBlockId) {
>      *aOutInputBlockId = block->GetBlockId();
>    }
>  
>    // Copy the event, since WheelBlockState needs to affix a counter.
>    ScrollWheelInput event(aEvent);

Do we need to copy the event twice? Can we unify this copy with the one made by the QueuedInput constructor?

::: gfx/layers/apz/src/InputQueue.cpp:335
(Diff revision 1)
>    if (!mInputBlockQueue.IsEmpty() &&
>        aEvent.mType != PanGestureInput::PANGESTURE_START) {
>      block = mInputBlockQueue.LastElement()->AsPanGestureBlock();
>    }
>  
>    PanGestureInput event = aEvent;

Likewise here, can we avoid the second copy?

::: gfx/layers/apz/src/InputQueue.cpp:386
(Diff revision 1)
>    // null) should take priority. This is equivalent to just always using the
>    // target (confirmed or not) from the block, which is what
>    // MaybeHandleCurrentBlock() does.
>    if (!MaybeHandleCurrentBlock(block, event)) {
>      block->AddEvent(event.AsPanGestureInput());
> +    mQueuedInputs.AppendElement(MakeUnique<QueuedInput>(event.AsPanGestureInput(), *block));

No need to call AsPanGestureInput().

	Botond Ballo [:botond]
	Comment 22 • 8 years ago
mozreview-review

Comment on attachment 8790525 [details]
Bug 1289432 - Start populating the mQueuedInput array with queued input objects.

https://reviewboard.mozilla.org/r/78300/#review77024

	Botond Ballo [:botond]
	Comment 23 • 8 years ago
mozreview-review

Comment on attachment 8790526 [details]
Bug 1289432 - Switch over to using the mActiveXXXBlock in some places instead of fishing around in mInputBlockQueue.

https://reviewboard.mozilla.org/r/78302/#review77026

	Botond Ballo [:botond]
	Comment 24 • 8 years ago
mozreview-review

Comment on attachment 8790523 [details]
Bug 1289432 - Have the InputQueue keep RefPtrs to the active block of each input type.

https://reviewboard.mozilla.org/r/78296/#review77028

::: gfx/layers/apz/src/InputQueue.cpp:727
(Diff revision 1)
>      }
>  
>      // If we get here, we know there are more touch blocks in the queue after
>      // |curBlock|, so we can remove |curBlock| and try to process the next one.
>      INPQ_LOG("discarding processed %s block %p\n", curBlock->Type(), curBlock);
> +    ClearActiveBlock(curBlock);

On second thought, I don't understand why it makes sense to clear the active block here.

Suppose you have an input queue with two touch blocks, A and B, with A having arrived first.

At this point, the active block is B.

If we now process A and remove it from the queue - shouldn't the active block remain B?

	Botond Ballo [:botond]
	Comment 25 • 8 years ago
mozreview-review

Comment on attachment 8790527 [details]
Bug 1289432 - Update checks for unprocessed events to use mQueuedInputs instead of checking the block is at the head.

https://reviewboard.mozilla.org/r/78304/#review77032

::: gfx/layers/apz/src/InputQueue.cpp:72
(Diff revision 1)
>  }
>  
>  bool
>  InputQueue::MaybeHandleCurrentBlock(CancelableBlockState *block,
>                                      const InputData& aEvent) {
> -  if (block == CurrentBlock() && block->IsReadyForHandling()) {
> +  if (mQueuedInputs.IsEmpty() && block->IsReadyForHandling()) {

Is this equivalent? What if the block is the current block, but it already has some inputs?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 26 • 8 years ago

(In reply to Botond Ballo [:botond] from comment #24)
> On second thought, I don't understand why it makes sense to clear the active
> block here.
> 
> Suppose you have an input queue with two touch blocks, A and B, with A
> having arrived first.
> 
> At this point, the active block is B.
> 
> If we now process A and remove it from the queue - shouldn't the active
> block remain B?

Yes. I think this is handled by the fact that ClearActiveBlock does checks if aBlock == mActiveTouchBlock. So in your example it would be A == B which would fail and mActiveTouchBlock would not get cleared.

(Also this code is cleaned up some more in a later patch)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 27 • 8 years ago

(In reply to Botond Ballo [:botond] from comment #25)
> > -  if (block == CurrentBlock() && block->IsReadyForHandling()) {
> > +  if (mQueuedInputs.IsEmpty() && block->IsReadyForHandling()) {
> 
> Is this equivalent? What if the block is the current block, but it already
> has some inputs?

If it already has some inputs, then we don't want to be dispatching aEvent right away (which is what that function does, by calling block->DispatchImmediate(aEvent). So in the scenario you describe we should actually return false without dispatching the event, and the mQueuedInputs.IsEmpty() check should ensure that. This function is also removed entirely in a later patch. :)

	Botond Ballo [:botond]
	Comment 28 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26)
> Yes. I think this is handled by the fact that ClearActiveBlock does checks
> if aBlock == mActiveTouchBlock. So in your example it would be A == B which
> would fail and mActiveTouchBlock would not get cleared.

You're right, of course. I need more coffee :)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27)
> (In reply to Botond Ballo [:botond] from comment #25)
> > > -  if (block == CurrentBlock() && block->IsReadyForHandling()) {
> > > +  if (mQueuedInputs.IsEmpty() && block->IsReadyForHandling()) {
> > 
> > Is this equivalent? What if the block is the current block, but it already
> > has some inputs?
> 
> If it already has some inputs, then we don't want to be dispatching aEvent
> right away (which is what that function does, by calling
> block->DispatchImmediate(aEvent).

I see, and DispatchImmediate() asserts !HasEvents(). If previously we would ever have gotten into the body of the if statement with the block already having events, that assertion would have fired anyways. LGTM then!

	Botond Ballo [:botond]
	Comment 29 • 8 years ago
mozreview-review

Comment on attachment 8790523 [details]
Bug 1289432 - Have the InputQueue keep RefPtrs to the active block of each input type.

https://reviewboard.mozilla.org/r/78296/#review77068

	Botond Ballo [:botond]
	Comment 30 • 8 years ago
mozreview-review

Comment on attachment 8790527 [details]
Bug 1289432 - Update checks for unprocessed events to use mQueuedInputs instead of checking the block is at the head.

https://reviewboard.mozilla.org/r/78304/#review77070

	Botond Ballo [:botond]
	Comment 31 • 8 years ago
mozreview-review

Comment on attachment 8790528 [details]
Bug 1289432 - Allow the CurrentBlock() and CurrentXXXBlock() functions to return null, and guard/assert at the callsites.

https://reviewboard.mozilla.org/r/78306/#review77072

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1100
(Diff revision 1)
>        mY.StartTouch(point.y, aEvent.mTime);
>        if (RefPtr<GeckoContentController> controller = GetGeckoContentController()) {
> +        MOZ_ASSERT(GetCurrentTouchBlock());
>          controller->NotifyAPZStateChange(
>              GetGuid(), APZStateChange::eStartTouch,
> -            CurrentTouchBlock()->GetOverscrollHandoffChain()->CanBePanned(this));
> +            GetCurrentTouchBlock()->GetOverscrollHandoffChain()->CanBePanned(this));

	Botond Ballo [:botond]
	Comment 32 • 8 years ago
mozreview-review

Comment on attachment 8790529 [details]
Bug 1289432 - Drop the DispatchImmediate codepath.

https://reviewboard.mozilla.org/r/78308/#review77086

	Botond Ballo [:botond]
	Comment 33 • 8 years ago
mozreview-review

Comment on attachment 8790530 [details]
Bug 1289432 - Stop relying on the mEvents array inside the SetConfirmedTargetApzc implementations.

https://reviewboard.mozilla.org/r/78310/#review77088

If we're getting a large volume of events per block (e.g. touch-moves or mouse-moves), could we be turning fast linear searches of blocks into slow linear searches of events?

::: gfx/layers/apz/src/InputQueue.h:184
(Diff revision 1)
> +   * corresponds to the block, or null if no such input was found. Note that
> +   * even if there are no inputs in mQueuedInputs, this function can return
> +   * non-null if the block id provided matches one of the depleted-but-still-
> +   * active blocks (mActiveTouchBlock, mActiveWheelBlock, etc.).
> +   */
> +  CancelableBlockState* FindBlockForId(const uint64_t& aInputBlockId,

Elsewhere we pass uint64_t by value. (Except in MainThreadTimeout(), I notice. Feel free to change that as well.)

::: gfx/layers/apz/src/InputQueue.cpp:554
(Diff revision 1)
>  
> +CancelableBlockState*
> +InputQueue::FindBlockForId(const uint64_t& aInputBlockId,
> +                           InputData** aOutFirstInput)
> +{
> +  for (size_t i = 0; i < mQueuedInputs.Length(); i++) {

for (const auto& input : mQueuedInputs) {
  if (input->Block()-> ...) {
    ...
  }
}

(|input| will have type |const UniquePtr<QueuedInput>&|. Or you can spell that out instead of |const auto&|, if you prefer.)

	Botond Ballo [:botond]
	Comment 34 • 8 years ago
mozreview-review

Comment on attachment 8790531 [details]
Bug 1289432 - Update a couple more functions to stop fishing around inside mInputBlockQueue.

https://reviewboard.mozilla.org/r/78312/#review77090

	Botond Ballo [:botond]
	Comment 35 • 8 years ago
mozreview-review

Comment on attachment 8790532 [details]
Bug 1289432 - Migrate remaining InputQueue code to use the new mQueuedInput structure instead of the block-based queue.

https://reviewboard.mozilla.org/r/78314/#review77106

::: gfx/layers/apz/src/InputQueue.cpp
(Diff revision 1)
>    } else if (mActivePanGestureBlock && mActivePanGestureBlock->GetBlockId() == aInputBlockId) {
>      block = mActivePanGestureBlock.get();
>    }
>    // Since we didn't encounter this block while iterating through mQueuedInputs,
>    // it must have no events associated with it at the moment.
> -  MOZ_ASSERT(!block || !block->HasEvents());

Why is this assertion being removed?

::: gfx/layers/apz/src/InputQueue.cpp:697
(Diff revision 1)
> +      !aBlock->IsReadyForHandling() ||
> +      aBlock->MustStayActive()) {
> +    return false;
> +  }
> +  InputData* firstInput = nullptr;
> +  FindBlockForId(aBlock->GetBlockId(), &firstInput);

(Again, are we potentially doing an excessive amount of linear-searching through events here?)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 36 • 8 years ago

(In reply to Botond Ballo [:botond] from comment #33)
> If we're getting a large volume of events per block (e.g. touch-moves or
> mouse-moves), could we be turning fast linear searches of blocks into slow
> linear searches of events?
> 

In theory, yes. However if we're getting a lot of events for the same block, then the search should end pretty quickly because most of the events will have the same block id, and the first one that matches will end the search. The scenario that would get slower is if we get a lot of input events for one block that queue up, and then while they're sitting in the queue we get a lot of input events for another block, and we have to search past the events from the first block in order to find the second block. I expect this scenario to be pretty rare, and overall I don't expect the perf impact to be noticeable since the iteration itself should be pretty quick.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 37 • 8 years ago

(In reply to Botond Ballo [:botond] from comment #35)
> >    // Since we didn't encounter this block while iterating through mQueuedInputs,
> >    // it must have no events associated with it at the moment.
> > -  MOZ_ASSERT(!block || !block->HasEvents());
> 
> Why is this assertion being removed?

Mostly because I removed the HasEvents() function. I could reimplement the HasEvents but it would just iterate through the queue again which we already did above in this function.

> ::: gfx/layers/apz/src/InputQueue.cpp:697
> > +  FindBlockForId(aBlock->GetBlockId(), &firstInput);
> 
> (Again, are we potentially doing an excessive amount of linear-searching
> through events here?)

Potentially, yeah. Again I think it probably won't be noticeable, same as comment 36.

	Botond Ballo [:botond]
	Comment 38 • 8 years ago
mozreview-review

Comment on attachment 8790533 [details]
Bug 1289432 - Miscellaneous function renaming and documentation touchups.

https://reviewboard.mozilla.org/r/78316/#review77112

Thanks for a very nicely broken up and easy to review patch series!

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 39 • 8 years ago

I addressed the remaining review comments (comment 21, comment 33) with an extra patch at the end of the series, since it avoided some painful rebasing, and in some places was modifying pre-existing code (such as the removal of many spurious AsTouchEvent() calls).

The one thing I didn't do was get rid of the extra PanGestureInput copy in ReceivePanGestureInput because doing that would require complicated code gymnastics in the function (or breaking the symmetry of the QueuedInput constructors) and I didn't feel it was worth it considering the extra copy is on the stack rather than the heap. The ScrollWheelInput copy was relatively trivial to remove by comparison. If there's a simple way to remove the PanGestureInput copy I can do it, but I didn't see anything obvious.

New try push just to double-check everything is sane: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6439938c33f2

	Botond Ballo [:botond]
	Comment 40 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #39)
> The one thing I didn't do was get rid of the extra PanGestureInput copy in
> ReceivePanGestureInput because doing that would require complicated code
> gymnastics in the function (or breaking the symmetry of the QueuedInput
> constructors)

Not sure if the attached patch constitutes code gymnastics - feel free to take it or leave it. (As you point out, it's just a stack copy so it's not a big deal.)

	Botond Ballo [:botond]
	Comment 41 • 8 years ago

Whoops, patdiff doesn't seem to work well for generating diff files.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 42 • 8 years ago

Comment on attachment 8790993 [details] [diff] [review]
Avoid an extra copy of a PanGestureInput

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

::: gfx/layers/apz/src/InputQueue.cpp
@@ +336,5 @@
> +  // turn this input into a PANGESTURE_START.
> +  if (event.mType != PanGestureInput::PANGESTURE_START) {
> +    INPQ_LOG("transmogrifying pan input %d to PANGESTURE_START for new block\n",
> +        event.mType);
> +    event.mType = PanGestureInput::PANGESTURE_START;

This looks like it will turn *all* PanGestureInput events into PANGESTURE_START events?

	Pulsebot
	Comment 43 • 8 years ago

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ff934bf3205
Use RefPtr instead of UniquePtr to hold the block state objects, since there will be multiple references to them soon. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/e03c153b2fe5
Have the InputQueue keep RefPtrs to the active block of each input type. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/73db06b5db9a
Introduce a skeleton QueuedInput class. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/5544fb9a3c9a
Start populating the mQueuedInput array with queued input objects. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e58cc81dbef
Switch over to using the mActiveXXXBlock in some places instead of fishing around in mInputBlockQueue. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/525c3eceb106
Update checks for unprocessed events to use mQueuedInputs instead of checking the block is at the head. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/4284c891813d
Allow the CurrentBlock() and CurrentXXXBlock() functions to return null, and guard/assert at the callsites. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/b40d59513ee6
Drop the DispatchImmediate codepath. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/868e5e9845ac
Stop relying on the mEvents array inside the SetConfirmedTargetApzc implementations. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff1c50ceb8b
Update a couple more functions to stop fishing around inside mInputBlockQueue. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/da3219c8f5b7
Migrate remaining InputQueue code to use the new mQueuedInput structure instead of the block-based queue. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/3644e59ed455
Miscellaneous function renaming and documentation touchups. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca284bdb324a
Miscellaneous tweaks to address review comments. rs=botond

	Botond Ballo [:botond]
	Comment 44 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #42)
> This looks like it will turn *all* PanGestureInput events into
> PANGESTURE_START events?

Indeed. That can be fixed, though it's looking more and more like "code gymnastics", so let's just leave it as is.

	Carsten Book [:Tomcat]
	Comment 45 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3ff934bf3205
https://hg.mozilla.org/mozilla-central/rev/e03c153b2fe5
https://hg.mozilla.org/mozilla-central/rev/73db06b5db9a
https://hg.mozilla.org/mozilla-central/rev/5544fb9a3c9a
https://hg.mozilla.org/mozilla-central/rev/1e58cc81dbef
https://hg.mozilla.org/mozilla-central/rev/525c3eceb106
https://hg.mozilla.org/mozilla-central/rev/4284c891813d
https://hg.mozilla.org/mozilla-central/rev/b40d59513ee6
https://hg.mozilla.org/mozilla-central/rev/868e5e9845ac
https://hg.mozilla.org/mozilla-central/rev/8ff1c50ceb8b
https://hg.mozilla.org/mozilla-central/rev/da3219c8f5b7
https://hg.mozilla.org/mozilla-central/rev/3644e59ed455
https://hg.mozilla.org/mozilla-central/rev/ca284bdb324a