Closed Bug 1451469 Opened 6 years ago Closed 6 years ago

Allow mapping the APZ sampler thread to the render backend thread

Categories

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

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(8 files)

59 bytes, text/x-review-board-request
botond
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
nical
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
nical
: review+
Details
Building on the changes in bug 1449982, we also need to map the APZ sampler thread to the render backend thread. That means running the APZ sampling functions (advance animations, get async scroll offset, etc.) on the render backend thread rather than on the compositor thread, which is where they happen now.
Comment on attachment 8967501 [details]
Bug 1451469 - Add the plumbing to hook up the sampler callbacks.

https://reviewboard.mozilla.org/r/236166/#review242166
Attachment #8967501 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8967504 [details]
Bug 1451469 - Add a TransactionWrapper class.

https://reviewboard.mozilla.org/r/236172/#review242168

nit: I'd call it something like TransactionRef since TransactionWrapper doesn't say much about ownership.
Comment on attachment 8967505 [details]
Bug 1451469 - Set the sample time on APZSampler.

https://reviewboard.mozilla.org/r/236174/#review242170
Attachment #8967505 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8967504 [details]
Bug 1451469 - Add a TransactionWrapper class.

https://reviewboard.mozilla.org/r/236172/#review242172
Attachment #8967504 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8967506 [details]
Bug 1451469 - Complete hooking up of the sampler thread.

https://reviewboard.mozilla.org/r/236176/#review242174
Attachment #8967506 - Flags: review?(nical.bugzilla) → review+
The posted patches include some small changes since the try push in comment 1, this is an updated push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1c5e98b231056fb82878ead63381711a992eed7
Comment on attachment 8967499 [details]
Bug 1451469 - Maintain a map from WrWindowId to APZSampler.

https://reviewboard.mozilla.org/r/236162/#review242364

::: gfx/layers/apz/public/APZSampler.h:102
(Diff revision 1)
> +
> +  // Used to manage the mapping from a WR window id to APZSampler. These are only
> +  // used if WebRender is enabled. Both sWindowIdMap and mWindowId should only
> +  // be used while holding the sWindowIdLock.
> +  static StaticMutex sWindowIdLock;
> +  static std::unordered_map<uint64_t, APZSampler*> sWindowIdMap;

Would it be simpler / more efficient to keep a single map from window id to {sampler, updater} pair?
Comment on attachment 8967500 [details]
Bug 1451469 - Implement the guts of the WR sampler thread registration.

https://reviewboard.mozilla.org/r/236164/#review242366
Attachment #8967500 - Flags: review?(botond) → review+
Comment on attachment 8967502 [details]
Bug 1451469 - Allow WR sampling without the tree lock.

https://reviewboard.mozilla.org/r/236168/#review242368

r+ with comments addressed.

::: commit-message-da7e1:6
(Diff revision 1)
> +Bug 1451469 - Allow WR sampling without the tree lock. r?botond
> +
> +For webrender, we need to be able to sample the async transforms from nodes
> +and thumbs without holding the tree lock, since the sampling happens on
> +the render backend thread, and holding the tree lock would be a
> +threading violation. We can use the mApzcMap to sample the node

Could we extend the comment near the top of APZCTreeManager.h to mention the thread ordering as well (basically, the last part of [1])?

[1] https://bug1391318.bmoattachments.org/attachment.cgi?id=8965040

::: gfx/layers/apz/src/APZCTreeManager.h:755
(Diff revision 1)
> +   */
> +  struct ScrollThumbInfo {
> +    uint64_t mThumbAnimationId;
> +    CSSTransformMatrix mThumbTransform;
> +    ScrollbarData mThumbData;
> +    ScrollableLayerGuid mTargetGuid;

Note that the target view id is stored in mThumbData; we could just store the layers id here, and have a getter that builds the guid.

I don't feel strongly about this, but if we stick with storing the guid, I'd like an assert in the constructor that the view id in mTargetGuid and the view id in mThumbData match.

::: gfx/layers/apz/src/APZCTreeManager.h:785
(Diff revision 1)
> +   * we cannot do on the WR sampler thread. mScrollThumbInfo, however, can
> +   * be accessed while just holding the mMapLock which is safe to do on the
> +   * sampler thread.
> +   * mMapLock must be acquired while accessing or modifying mScrollThumbInfo.
> +   */
> +  std::vector<ScrollThumbInfo> mScrollThumbInfo;

I don't love the duplication of data here, but I suppose there are relatively few scrollbar thumbs in the display list at a given time, so it's not a big deal.

::: gfx/layers/apz/src/APZCTreeManager.cpp:121
(Diff revision 1)
> +  std::vector<HitTestingTreeNode*> mScrollThumbs;
> +  // This is populated with all the scroll target nodes
> +  std::unordered_map<ScrollableLayerGuid,
> +                     HitTestingTreeNode*,
> +                     ScrollableLayerGuid::HashIgnoringPresShellFn,
> +                     ScrollableLayerGuid::EqualIgnoringPresShellFn> mThumbTargets;

The name mThumbTargets is a bit confusing, because it suggests that the map only stores information pertaining to thumbs, when it fact it stores all scrollable nodes (as the comment says).

The duplication with mApzcMap is a bit unfortunate (as in, we can _almost_ just use mApzcMap, but not quite), but I don't really have any simple suggestions for how to avoid it.

::: gfx/layers/apz/src/APZCTreeManager.cpp:421
(Diff revision 1)
> +          // GetScrollbarAnimationId is only non-zero when webrender is enabled,
> +          // which limits the extra thumb mapping work to the webrender-enabled
> +          // case where it is needed.
> +          // Note also that when webrender is enabled, a "valid" animation id
> +          // is always nonzero, so we don't need to worry about handling the
> +          // case where WR is enabled and the animation is zero.

"animation id is zero"

::: gfx/layers/apz/src/APZCTreeManager.cpp:518
(Diff revision 1)
> +    for (HitTestingTreeNode* thumb : state.mScrollThumbs) {
> +      MOZ_ASSERT(thumb->IsScrollThumbNode());
> +      ScrollableLayerGuid targetGuid(thumb->GetLayersId(), 0, thumb->GetScrollTargetId());
> +      auto it = state.mThumbTargets.find(targetGuid);
> +      if (it == state.mThumbTargets.end()) {
> +        continue;

The comment "A scrollbar for content which didn't have an APZC. ..." (which I ask you to preserve below) would also be appropriate here.

::: gfx/layers/apz/src/APZCTreeManager.cpp:596
(Diff revision 1)
> -                     HitTestingTreeNode*,
> -                     ScrollableLayerGuid::HashIgnoringPresShellFn,
> -                     ScrollableLayerGuid::EqualIgnoringPresShellFn> httnMap;
> -
>    bool activeAnimations = false;
> -  LayersId lastLayersId{(uint64_t)-1};
> +  for (const auto& i : mApzcMap) {

'i' is a pair, not an iterator or an index. I would call it something like 'element' or 'mapping'.

::: gfx/layers/apz/src/APZCTreeManager.cpp:608
(Diff revision 1)
> -        // the scroll offset. Since we are effectively giving WR the async
> +    // the scroll offset. Since we are effectively giving WR the async
> -        // scroll delta here, we want to negate the translation.
> +    // scroll delta here, we want to negate the translation.
> -        ParentLayerPoint asyncScrollDelta = -layerTranslation;
> +    ParentLayerPoint asyncScrollDelta = -layerTranslation;
> -        // XXX figure out what zoom-related conversions need to happen here.
> +    // XXX figure out what zoom-related conversions need to happen here.
> -        aTxn.UpdateScrollPosition(lastPipelineId, apzc->GetGuid().mScrollId,
> +    aTxn.UpdateScrollPosition(
> +        wr::AsPipelineId(apzc->GetGuid().mLayersId),

If a pipeine id can be derived from a layers id using call to wr::AsPipelineId(), why were we going to the trouble of looking up the pipeline id in the LayerTreeState before?

::: gfx/layers/apz/src/APZCTreeManager.cpp:621
(Diff revision 1)
> -  // resulting in O(n log n) runtime.
>    nsTArray<wr::WrTransformProperty> scrollbarTransforms;
> -  ForEachNode<ReverseIterator>(mRootNode.get(),
> -      [&](HitTestingTreeNode* aNode)
> -      {
> -        if (!aNode->IsScrollThumbNode()) {
> +  for (const ScrollThumbInfo& info : mScrollThumbInfo) {
> +    auto it = mApzcMap.find(info.mTargetGuid);
> +    if (it == mApzcMap.end()) {
> +      continue;

Please preserve the comment "A scrollbar for content  which didn't have an APZC. ..."
Attachment #8967502 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #16)
> > +  // be used while holding the sWindowIdLock.
> > +  static StaticMutex sWindowIdLock;
> > +  static std::unordered_map<uint64_t, APZSampler*> sWindowIdMap;
> 
> Would it be simpler / more efficient to keep a single map from window id to
> {sampler, updater} pair?

It would use less memory and initialization would probably be a bit simpler. However, we do lookups in the map from different threads anyway, so I don't think lookup will be made any more efficient or simpler. The tradeoff is that we'd want to put the map somewhere else (maybe in WebRenderAPI.cpp?) and so it becomes less encapsulated as an APZ thing.

Also note that currently entries are removed from the map at different points - APZUpdater clears it in the ClearTree runnable while APZSampler clears it in the destructor. If we had a map with a pair we'd probably want to clear the pair from CompositorBridgeParent::StopAndClearResources or similar (i.e. in the code that's being moved in bug 1454430), but that runs before the ClearTree runnable actually executes, and so might happen too early. Or we could clear either half of the pair separately, but that seems more convoluted.

I can try to find a cleaner way to do this if you want, but at the moment just thinking about it I don't think that the pair would result in overall cleaner code.

(In reply to Botond Ballo [:botond] from comment #18)
> Comment on attachment 8967502 [details]
> Bug 1451469 - Allow WR sampling without the tree lock.

I agree with all your comments on this patch, I'll make the necessary changes.
Comment on attachment 8967503 [details]
Bug 1451469 - Run deferred tasks on the controller thread.

https://reviewboard.mozilla.org/r/236170/#review242728

::: commit-message-9e459:5
(Diff revision 1)
> +Bug 1451469 - Run deferred tasks on the controller thread. r?botond
> +
> +Deferred tasks currently run as part of the sampling process, in
> +AdvanceAnimations. However, deferred tasks also sometimes acquire the
> +APZ tree lock for stuff. Acquiring the tree lock is not going to be

"acquire the APZ tree lock for stuff" -> "need to acquire the APZ tree lock" :)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3493
(Diff revision 1)
>    // This function may get called multiple with the same sample time, because
>    // there may be multiple layers with this APZC, and each layer invokes this
>    // function during composition. However we only want to do one animation step
>    // per composition so we need to deduplicate these calls first.
>    if (mLastSampleTime == aSampleTime) {
> -    return false;
> +    return (mAnimation != nullptr);

from IRC: this change should go in part 7 along with an update to the comment that says "we might get called multiple times with the same timestamp"

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
(Diff revision 1)
> -    deferredTasks[i] = nullptr;
>    }
>  
> -  // One of the deferred tasks may have started a new animation. In this case,
> -  // we want to ask the compositor to schedule a new composite.
> +  // If any of the deferred tasks starts a new animation, it will request a
> +  // new composite directly, so we can just return requestAnimationFrame here.
> -  requestAnimationFrame |= (mAnimation != nullptr);

Pretty sure this change would break APZCBasicTester.FlingIntoOverscroll if gtests were run with the sampler thread != controller thread, because nothing would run the task.

Come to think of it, a number of gtests might break in such a configuration for that reason. Not sure how much we care about that...
Attachment #8967503 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #20)
> > -  // One of the deferred tasks may have started a new animation. In this case,
> > -  // we want to ask the compositor to schedule a new composite.
> > +  // If any of the deferred tasks starts a new animation, it will request a
> > +  // new composite directly, so we can just return requestAnimationFrame here.
> > -  requestAnimationFrame |= (mAnimation != nullptr);
> 
> Pretty sure this change would break APZCBasicTester.FlingIntoOverscroll if
> gtests were run with the sampler thread != controller thread, because
> nothing would run the task.

Sorry, this comment is a bit misleadingly placed. It's not the removal of this line that would break the test in such a scenario, but actually running the task on the controller thread.
Comment on attachment 8967499 [details]
Bug 1451469 - Maintain a map from WrWindowId to APZSampler.

https://reviewboard.mozilla.org/r/236162/#review242772

Let's just go with the two maps, it doesn't seem to be worth the trouble to combine them.
Attachment #8967499 - Flags: review?(botond) → review+
Comment on attachment 8967505 [details]
Bug 1451469 - Set the sample time on APZSampler.

https://reviewboard.mozilla.org/r/236174/#review242778

It would feel more natural to me if WRBP told WR about the composite time, and WR then passed it in via the callback, but I don't feel strongly about that.

Also, and I realize that this is a pre-existing problem, but what if the rendering takes a long time and we miss the next vsync?

::: gfx/layers/apz/src/APZSampler.cpp:89
(Diff revision 1)
> +    // WebRenderBridgeParent hasn't yet provided us with a sample time.
> +    // If we're that early there probably aren't any APZ animations happening
> +    // anyway, so using Timestamp::Now() should be fine.
> +    sampleTime = mSampleTime.IsNull() ? TimeStamp::Now() : mSampleTime;
> +  }
> +  return mApz->SampleForWebRender(aTxn, sampleTime);

nit: patch splitting error, SampleForWebRender isn't renamed until the next patch
Attachment #8967505 - Flags: review?(botond) → review+
Comment on attachment 8967506 [details]
Bug 1451469 - Complete hooking up of the sampler thread.

https://reviewboard.mozilla.org/r/236176/#review242782
Attachment #8967506 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #18)
> Could we extend the comment near the top of APZCTreeManager.h to mention the
> thread ordering as well (basically, the last part of [1])?
> 
> [1] https://bug1391318.bmoattachments.org/attachment.cgi?id=8965040

Done

> ::: gfx/layers/apz/src/APZCTreeManager.h:755
> > +    ScrollbarData mThumbData;
> > +    ScrollableLayerGuid mTargetGuid;
> 
> Note that the target view id is stored in mThumbData; we could just store
> the layers id here, and have a getter that builds the guid.
> 
> I don't feel strongly about this, but if we stick with storing the guid, I'd
> like an assert in the constructor that the view id in mTargetGuid and the
> view id in mThumbData match.

I put an assert in the constructor for now, and filed bug 1454485 for follow-up to remove the duplication and clean up other omissions I found (and that we discussed on IRC).

> ::: gfx/layers/apz/src/APZCTreeManager.h:785
> The name mThumbTargets is a bit confusing, because it suggests that the map
> only stores information pertaining to thumbs, when it fact it stores all
> scrollable nodes (as the comment says).

I renamed it to mScrollTargets and expanded the comment a bit to indicate it's used for thumb stuff.

> The duplication with mApzcMap is a bit unfortunate (as in, we can _almost_
> just use mApzcMap, but not quite), but I don't really have any simple
> suggestions for how to avoid it.

Yeah :/

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:421
> > +          // case where WR is enabled and the animation is zero.
> 
> "animation id is zero"

Fixed

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:518
> > +      auto it = state.mThumbTargets.find(targetGuid);
> > +      if (it == state.mThumbTargets.end()) {
> > +        continue;
> 
> The comment "A scrollbar for content which didn't have an APZC. ..." (which
> I ask you to preserve below) would also be appropriate here.

Done. I reworded the comment slightly to refer to the local variables in context but the essence is the same.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:596
> > +  for (const auto& i : mApzcMap) {
> 
> 'i' is a pair, not an iterator or an index. I would call it something like
> 'element' or 'mapping'.

Renamed to mapping.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:608
> If a pipeine id can be derived from a layers id using call to
> wr::AsPipelineId(), why were we going to the trouble of looking up the
> pipeline id in the LayerTreeState before?

I think mostly because when I wrote that code I was being stupid, or didn't realize that I could just convert the layers id to a pipeline id.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:621
> > +    if (it == mApzcMap.end()) {
> > +      continue;
> 
> Please preserve the comment "A scrollbar for content  which didn't have an
> APZC. ..."

Done

(In reply to Botond Ballo [:botond] from comment #20)
> "acquire the APZ tree lock for stuff" -> "need to acquire the APZ tree lock"
> :)

Done

> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3493
> > -    return false;
> > +    return (mAnimation != nullptr);
> 
> from IRC: this change should go in part 7 along with an update to the
> comment that says "we might get called multiple times with the same
> timestamp"

Fixed

> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp
> Pretty sure this change would break APZCBasicTester.FlingIntoOverscroll if
> gtests were run with the sampler thread != controller thread, because
> nothing would run the task.
> 
> Come to think of it, a number of gtests might break in such a configuration
> for that reason. Not sure how much we care about that...

Yeah, I'm quite happy that the gtests are single-threaded :)

(In reply to Botond Ballo [:botond] from comment #23)
> It would feel more natural to me if WRBP told WR about the composite time,
> and WR then passed it in via the callback, but I don't feel strongly about
> that.

I agree, that would feel more natural to me as well. But I want to check with nical (probably on bug 1453360) what his expectation is with respect to compositing during long scene builds.

> Also, and I realize that this is a pre-existing problem, but what if the
> rendering takes a long time and we miss the next vsync?

Not totally sure I understand the question. If rendering takes more than 16ms then we can't maintain 60fps by definition. In this case we'd get (say) two vsyncs during the long render. The timestamp of the second vsync would clobber the timestamp of the first vsync, so at the next sampling/render we'd use the more recent timestamp. Does that answer your question?

> ::: gfx/layers/apz/src/APZSampler.cpp:89
> > +  return mApz->SampleForWebRender(aTxn, sampleTime);
> 
> nit: patch splitting error, SampleForWebRender isn't renamed until the next
> patch

Fixed
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25)
> > Also, and I realize that this is a pre-existing problem, but what if the
> > rendering takes a long time and we miss the next vsync?
> 
> Not totally sure I understand the question. If rendering takes more than
> 16ms then we can't maintain 60fps by definition. In this case we'd get (say)
> two vsyncs during the long render. The timestamp of the second vsync would
> clobber the timestamp of the first vsync, so at the next sampling/render
> we'd use the more recent timestamp. Does that answer your question?

Yep, that sounds reasonable. (In fact, that may be an argument for having WRBP tell APZ the timestamp directly, to avoid using a stale timestamp taken when the rendering was started?)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25)
> I put an assert in the constructor for now, and filed bug 1454485 for
> follow-up to remove the duplication and clean up other omissions I found
> (and that we discussed on IRC).

This assertion fails in the try push above, probably because of the broken behaviour described in bug 1454485. I'll write a patch for that bug and see if it fixes the issue.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78d008b79f62
Maintain a map from WrWindowId to APZSampler. r=botond
https://hg.mozilla.org/integration/autoland/rev/baf94bbd3243
Implement the guts of the WR sampler thread registration. r=botond
https://hg.mozilla.org/integration/autoland/rev/32bcab2ef2e5
Add the plumbing to hook up the sampler callbacks. r=nical
https://hg.mozilla.org/integration/autoland/rev/e9754863da62
Allow WR sampling without the tree lock. r=botond
https://hg.mozilla.org/integration/autoland/rev/4ed9f2bddf7f
Run deferred tasks on the controller thread. r=botond
https://hg.mozilla.org/integration/autoland/rev/82ed4ba5a5e8
Add a TransactionWrapper class. r=nical
https://hg.mozilla.org/integration/autoland/rev/61724e1bb2ad
Set the sample time on APZSampler. r=botond,nical
https://hg.mozilla.org/integration/autoland/rev/7ccbfd4a28d7
Complete hooking up of the sampler thread. r=botond,nical
You need to log in before you can comment on or make changes to this bug.