Closed Bug 1447299 Opened 6 years ago Closed 6 years ago

Support having a different sampler thread per APZSampler/APZCTreeManager

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(6 files, 6 obsolete files)

Bug 1447299 - Have the APZCTreeManager keep a pointer to the sampler.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1447299 - Move the sampler thread util functions from APZThreadUtils to APZSampler.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1447299 - Move the RunOnControllerThread from APZCTreeManager::SetLongTapEnabled to the caller.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1447299 - Have the APZCTreeManagerParent store a reference to the APZSampler.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1447299 - Bounce PAPZCTreeManager controller messages through the sampler thread.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1447299 - Ensure all APZSampler functions run on the sampler thread.
59 bytes, text/x-review-board-request
botond
: review+
Details 
When I wrote the patches for bug 1441916 I failed to take into account that we actually have a separate render backend thread (and therefore will have a separate sampler thread) for each open window. So each APZSampler will need to track its own sampler thread, rather than having static sampler thread stuff in APZThreadUtils.
Priority: -- → P3
Whiteboard: [gfx-noted]
I discovered that we'll probably need to make the scene builder thread the sampler thread, rather than the render backend thread as we had originally planned (see bug 1391318 comment 10). However I'm not 100% confident on that yet. Regardless of which thread we use, it's still going to be a different thread per APZSampler/APZCTreeManager so we'll need to make sure all the thread utils functions pertaining to the sampler thread are per-sampler and not static. As my local patch queue is getting too large for my liking, I'm going to extract the patches relevant to that change and post them on this bug while I continue figuring out the rest of it.
Comment on attachment 8961551 [details]
Bug 1447299 - Have the APZCTreeManager keep a pointer to the sampler.

https://reviewboard.mozilla.org/r/230340/#review236158

::: gfx/layers/apz/src/APZCTreeManager.h:698
(Diff revision 1)
>    /* Layers id for the root CompositorBridgeParent that owns this APZCTreeManager. */
>    uint64_t mRootLayersId;
>  
> +  /* Pointer to the APZSampler instance that is bound to this APZCTreeManager.
> +   * The sampler has a RefPtr to this class that is not nulled out until it
> +   * is destroyed, so we keep a non-owning raw pointer back to the APZSampler.

I don't see how the fact that APZSampler has a RefPtr to APZCTreeManager that's not nulled out until APZSampler is destroyed, ensures that APZSampler lives as long as APZCTreeManager.

What's to stop someone from doing:

\{  // some outer scope
  RefPtr<APZCTreeManager> tm = new APZCTreeManager(...);

  \{  // some inner scope
    RefPtr<APZSampler> sampler = new APZSampler(tm);
    tm->SetSampler(sampler.get());

    // ...

  \}

  tm->AnyMethodThatAssertsOnSamplerThread();  // oops
\}
Comment on attachment 8961552 [details]
Bug 1447299 - Move the sampler thread util functions from APZThreadUtils to APZSampler.

https://reviewboard.mozilla.org/r/230342/#review236142

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3482
(Diff revision 1)
>  }
>  
>  bool AsyncPanZoomController::UpdateAnimation(const TimeStamp& aSampleTime,
>                                               nsTArray<RefPtr<Runnable>>* aOutDeferredTasks)
>  {
> -  APZThreadUtils::AssertOnSamplerThread();
> +  if (APZCTreeManager* treeManagerLocal = GetApzcTreeManager()) {

Extract into a helper function?
Attachment #8961552 - Flags: review?(botond) → review+
Comment on attachment 8961553 [details]
Bug 1447299 - Move the RunOnControllerThread from APZCTreeManager::SetLongTapEnabled to the caller.

https://reviewboard.mozilla.org/r/230344/#review236162
Attachment #8961553 - Flags: review?(botond) → review+
Comment on attachment 8961554 [details]
Bug 1447299 - Have the APZCTreeManagerParent store a reference to the APZSampler.

https://reviewboard.mozilla.org/r/230346/#review236166

::: gfx/layers/ipc/APZCTreeManagerParent.cpp:20
(Diff revision 1)
>  
> -APZCTreeManagerParent::APZCTreeManagerParent(uint64_t aLayersId, RefPtr<APZCTreeManager> aAPZCTreeManager)
> +APZCTreeManagerParent::APZCTreeManagerParent(uint64_t aLayersId,
> +                                             RefPtr<APZCTreeManager> aAPZCTreeManager,
> +                                             RefPtr<APZSampler> aAPZSampler)
>    : mLayersId(aLayersId)
>    , mTreeManager(aAPZCTreeManager)

I overlooked this last time, but we should Move() the incoming RefPtrs to avoid refcount churn (and the phrase the assertions in terms of the m variables).

::: gfx/layers/ipc/APZCTreeManagerParent.cpp:39
(Diff revision 1)
> +                                    RefPtr<APZSampler> aAPZSampler)
>  {
>    MOZ_ASSERT(aAPZCTreeManager != nullptr);
> +  MOZ_ASSERT(aAPZSampler != nullptr);
> +  MOZ_ASSERT(aAPZSampler->HasTreeManager(aAPZCTreeManager));
>    mTreeManager = aAPZCTreeManager;

Likewise (here the assertions are fine as-is since we're asserting first, then moving)
Attachment #8961554 - Flags: review?(botond) → review+
Comment on attachment 8961555 [details]
Bug 1447299 - Bounce PAPZCTreeManager controller messages through the sampler thread.

https://reviewboard.mozilla.org/r/230348/#review236174

I had some concerns about the overhead of excessive bouncing which we discussed on IRC. I'm happy to proceed with this for now, and keep an eye out for potential performance issues and address them as they come up.

::: commit-message-deb84:9
(Diff revision 1)
> +compositor thread, which currently is always the same as the sampler
> +thread. When it does RunOnControllerThread calls, there is an implicit
> +ordering with respect to the sampler thread, because the controller
> +thread messages are always dispatched *from* the sampler thread. When we
> +move the sampler thread to be the render backend thread, we have to
> +preseve this implicit ordering, but we have to make it more explicit.

It wasn't clear to me what the ordering issue was until you explained it on IRC. Could you revise this message to make it more clear? Feel free to mention an example like the layers update / SetTargetAPZC one.
Attachment #8961555 - Flags: review?(botond) → review+
Comment on attachment 8961556 [details]
Bug 1447299 - Ensure all APZSampler functions run on the sampler thread.

https://reviewboard.mozilla.org/r/230350/#review236184

::: gfx/layers/apz/src/APZSampler.cpp:147
(Diff revision 1)
>  {
>    MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
> -  return mApz->GetAPZTestData(aLayersId, aOutData);
> +
> +  RefPtr<APZCTreeManager> apz = mApz;
> +  bool ret = false;
> +  SynchronousTask waiter("APZSampler::GetAPZTestData");

Neat utility :)

The comment above SynchronousTask's definition says "This can go away when we switch ImageBridge to an XPCOM thread". Assuming that's no longer the case, please remove that comment.

::: gfx/layers/apz/src/APZSampler.cpp:168
(Diff revision 1)
>  APZSampler::SetTestAsyncScrollOffset(uint64_t aLayersId,
>                                       const FrameMetrics::ViewID& aScrollId,
>                                       const CSSPoint& aOffset)
>  {
>    MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
> -  RefPtr<AsyncPanZoomController> apzc = mApz->GetTargetAPZC(aLayersId, aScrollId);
> +  RefPtr<APZSampler> self = this;

(Any reason's we're keeping a ref to the APZSampler and not the APZCTreeManager as in GetAPZTestData?)
Attachment #8961556 - Flags: review?(botond) → review+
Comment on attachment 8961551 [details]
Bug 1447299 - Have the APZCTreeManager keep a pointer to the sampler.

https://reviewboard.mozilla.org/r/230340/#review236700

(Dropping review flag until comment 10 is addressed.)
Attachment #8961551 - Flags: review?(botond)
Attachment #8961551 - Attachment is obsolete: true
Attachment #8961552 - Attachment is obsolete: true
Attachment #8961553 - Attachment is obsolete: true
Attachment #8961554 - Attachment is obsolete: true
Attachment #8961555 - Attachment is obsolete: true
Attachment #8961556 - Attachment is obsolete: true
(In reply to Botond Ballo [:botond] from comment #10)
> I don't see how the fact that APZSampler has a RefPtr to APZCTreeManager
> that's not nulled out until APZSampler is destroyed, ensures that APZSampler
> lives as long as APZCTreeManager.

I cleared out the pointer in APZCTM during APZSampler destruction like you suggested. In practice the lifetimes of the two should be the same.

(In reply to Botond Ballo [:botond] from comment #11)
> > +  if (APZCTreeManager* treeManagerLocal = GetApzcTreeManager()) {
> 
> Extract into a helper function?

Done

(In reply to Botond Ballo [:botond] from comment #13) 
> I overlooked this last time, but we should Move() the incoming RefPtrs to
> avoid refcount churn (and the phrase the assertions in terms of the m
> variables).

Done

> ::: gfx/layers/ipc/APZCTreeManagerParent.cpp:39
> Likewise (here the assertions are fine as-is since we're asserting first,
> then moving)

Done

(In reply to Botond Ballo [:botond] from comment #14)
> 
> It wasn't clear to me what the ordering issue was until you explained it on
> IRC. Could you revise this message to make it more clear? Feel free to
> mention an example like the layers update / SetTargetAPZC one.

Updated commit message accordingly

(In reply to Botond Ballo [:botond] from comment #15)
> The comment above SynchronousTask's definition says "This can go away when
> we switch ImageBridge to an XPCOM thread". Assuming that's no longer the
> case, please remove that comment.

Yup, done

> ::: gfx/layers/apz/src/APZSampler.cpp:168
> > -  RefPtr<AsyncPanZoomController> apzc = mApz->GetTargetAPZC(aLayersId, aScrollId);
> > +  RefPtr<APZSampler> self = this;
> 
> (Any reason's we're keeping a ref to the APZSampler and not the
> APZCTreeManager as in GetAPZTestData?)

Nope, fixed. Also fixed in SetTestAsyncZoom
MozReview seems to have gotten confused and didn't realize that these are updated versions of the previous patches...

I assume you meant to carry the r+ for all the patches except the first?
Doh, I didn't even notice. Yes, carry r+ on all except the first. Thanks!
Comment on attachment 8962564 [details]
Bug 1447299 - Have the APZCTreeManager keep a pointer to the sampler.

https://reviewboard.mozilla.org/r/231368/#review237612

I'd like it slightly better if there were a GetSampler() function that did a MOZ_ASSERT, and all accesses to the sampler went through that.

::: gfx/layers/apz/src/APZCTreeManager.h:698
(Diff revision 1)
>  private:
>    /* Layers id for the root CompositorBridgeParent that owns this APZCTreeManager. */
>    LayersId mRootLayersId;
>  
> +  /* Pointer to the APZSampler instance that is bound to this APZCTreeManager.
> +   * The sampler has a RefPtr to this class that is not nulled out until it

The "that is not nulled out until it is destroyed" part is still not relevant. (On the other hand, mentioning that the APZSampler destructor nulls out this mSampler field would be relevant.)

::: gfx/layers/apz/src/APZCTreeManager.cpp:257
(Diff revision 1)
>  
>  void
> +APZCTreeManager::SetSampler(APZSampler* aSampler)
> +{
> +  // We're either setting the sampler or clearing it
> +  MOZ_ASSERT((mSampler == nullptr) ^ (aSampler == nullptr));

We're performing a logical comparison, not combining bits, so please use != instead of ^.
Attachment #8962564 - Flags: review?(botond) → review+
Comment on attachment 8962565 [details]
Bug 1447299 - Move the sampler thread util functions from APZThreadUtils to APZSampler.

https://reviewboard.mozilla.org/r/231370/#review237622
Attachment #8962565 - Flags: review?(botond) → review+
Comment on attachment 8962566 [details]
Bug 1447299 - Move the RunOnControllerThread from APZCTreeManager::SetLongTapEnabled to the caller.

https://reviewboard.mozilla.org/r/231372/#review237624
Attachment #8962566 - Flags: review?(botond) → review+
Comment on attachment 8962567 [details]
Bug 1447299 - Have the APZCTreeManagerParent store a reference to the APZSampler.

https://reviewboard.mozilla.org/r/231374/#review237626
Attachment #8962567 - Flags: review?(botond) → review+
Comment on attachment 8962568 [details]
Bug 1447299 - Bounce PAPZCTreeManager controller messages through the sampler thread.

https://reviewboard.mozilla.org/r/231376/#review237628
Attachment #8962568 - Flags: review?(botond) → review+
Comment on attachment 8962569 [details]
Bug 1447299 - Ensure all APZSampler functions run on the sampler thread.

https://reviewboard.mozilla.org/r/231378/#review237630
Attachment #8962569 - Flags: review?(botond) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df0e43f7c1ff5fd3a647a0fbb5451d168005d657 has the comments addressed and rebased to current master. I'll rebase to autoland tip for landing, because otherwise it'll probably conflict with bug 1449145 which just landed.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b4f127d99ca
Have the APZCTreeManager keep a pointer to the sampler. r=botond
https://hg.mozilla.org/integration/autoland/rev/e76ec9b09021
Move the sampler thread util functions from APZThreadUtils to APZSampler. r=botond
https://hg.mozilla.org/integration/autoland/rev/2c7c644cdc1c
Move the RunOnControllerThread from APZCTreeManager::SetLongTapEnabled to the caller. r=botond
https://hg.mozilla.org/integration/autoland/rev/1e39a095860a
Have the APZCTreeManagerParent store a reference to the APZSampler. r=botond
https://hg.mozilla.org/integration/autoland/rev/cac0b3a18cf9
Bounce PAPZCTreeManager controller messages through the sampler thread. r=botond
https://hg.mozilla.org/integration/autoland/rev/720ac5310fb4
Ensure all APZSampler functions run on the sampler thread. r=botond
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: