1449982 - Allow mapping the APZ updater thread to the WR scene builder thread

Status: RESOLVED FIXED

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

Description

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

As discussed somewhat in bug 1391318, we need to apply APZ updates for a particular content transaction at the same time that WR swaps in the corresponding built scene coming from the scene builder thread. And in particular, we need to acquire the APZ tree lock while we do both the scene swap and the APZ update, and then release the lock when done. (This is so that any hit-testing that is happening concurrently always get consistent results).

And also, we can't acquire the APZ tree lock on the render backend thread because that would result in a deadlock situation. So we have to do it from the scene builder thread. This means that when WR is enabled with async scene building, the scene builder thread has to serve as the APZ updater thread (which is an abstract concept introduced in bug 1449620).

This bug will do all the plumbing to hook this up.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9952ed3949849d7ba3204bbddf4028ef5e44c585

Need to debug this, it's not working as expected when the async scene building is turned on.

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=631461e4752531e4b72b1a95c0ab50d1713a6e98

Now the code is mostly behaving as I want it to. When I turn it on there's some flickering which seems to be due to the async scroll offsets not getting applied on some frames. I don't think it's a blocker to getting these patches landed since it's off by default anyway and again my patch queue is getting too large to manage.

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dfe1478092b32844b9d6ed5010bf31bf3008702

Now with the build bustage fixed, hopefully.

Comment 4

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

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

This should fix most/all of the orange

Comment 5

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

Attachment: Bug 1449982 - Don't hold the sIndirectLayerTreesLock unnecessarily while notifying APZ of a layer tree removal.

Review commit: https://reviewboard.mozilla.org/r/234090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234090/

Comment 6

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

Attachment: Bug 1449982 - Move the window id allocation to CompositorBridgeParent.

This includes a signature change from CompositorBridgeParentBase ->
CompositorBridgeParent which is not strictly required, but it makes it less
likely that we'll accidentally create a WebRenderAPI from somewhere else
and pass a nonsense window id. In effect, the signature change makes it
likely that only CompositorBridgeParent will allocate the window ids.

Review commit: https://reviewboard.mozilla.org/r/234092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234092/

Comment 7

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

Attachment: Bug 1449982 - Maintain a map from WrWindowId to APZUpdater.

This will allow callbacks from rust code to get a handle to the
necessary APZUpdater instance on which to invoke functions.

Review commit: https://reviewboard.mozilla.org/r/234094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234094/

Comment 8

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

Attachment: Bug 1449982 - Introduce pref to control async scene building.

Until all the pieces are in place, turning on this pref will probably
break horribly. But we need the pref so we can add the rest of the
pieces with enabling them by default.

Review commit: https://reviewboard.mozilla.org/r/234096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234096/

Comment 9

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

Attachment: Bug 1449982 - Add the plumbing for scene builder thread interaction.

This is all the (bidirectional) glue that connects the SceneBuilderHooks
to the APZUpdater.

Review commit: https://reviewboard.mozilla.org/r/234098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234098/

Comment 10

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

Attachment: Bug 1449982 - Implement the WR updater thread registration.

This lets the APZUpdater know which thread is the actual updater thread.
This is only really used for the thread assertions, but might be useful
for debugging and such as well.

Review commit: https://reviewboard.mozilla.org/r/234100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234100/

Comment 11

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

Attachment: Bug 1449982 - Add the task queue for running things on the updater thread.

If we're using the WR scene builder thread as the updater thread, we
cannot just post a task to its message loop, because it's a rust thread
doesn't have a message loop. Instead, we put these tasks into a queue
that we will process in callbacks that are invoked from the updater
thread.

This patch just adds the basic queue machinery, it will get more
complicated in a future patch because we need to gate the tasks to make
sure they don't run too early.

Review commit: https://reviewboard.mozilla.org/r/234102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234102/

Comment 12

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

Attachment: Bug 1449982 - Clean up WrEpoch usage.

I don't know why we keep using plain uint32_t and uint64_t values when
we have better types that we can use. This makes the code use and store
wr::Epoch natively instead of raw uint32_t values that are wrapped
on-demand.

Review commit: https://reviewboard.mozilla.org/r/234104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234104/

Comment 13

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

Attachment: Bug 1449982 - Move the WebRenderScrollData storage from WebRenderBridgeParent to APZUpdater.

This allows us to easily store a handle to the APZUpdater on the
WebRenderScrollDataWrapper class and walk around in the scroll data tree
without having to query other classes like CompositorBridgeParent or
WebRenderBridgeParent when we encounter a reflayer boundary.

Review commit: https://reviewboard.mozilla.org/r/234106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234106/

Comment 14

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

Attachment: Bug 1449982 - Hook up epoch-based task processing on the updater thread.

This makes it so that we only process a UpdateHitTestingTree task (any
tasks queued after it) once we know that the corresponding scene has
been built and swapped in by WebRender. This ensures that processing of
APZ data stays in sync with the active scene in WR.

The way we do this is by tracking the "epoch" (which is updated per
WR/layers transaction) that the APZ messages are based on. Only once the
scene for that transaction is swapped in do we process those messages.

Review commit: https://reviewboard.mozilla.org/r/234108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234108/

Comment 15

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5641d1154273e4dfdf580201a5b9f9d5e27ad496

Comment 16

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> When I turn it on there's
> some flickering which seems to be due to the async scroll offsets not
> getting applied on some frames.

I'm fairly sure this is because the async scroll offsets are currently being sent to WR as part of the "GenerateFrame" transaction at [1] and therefore races with the scene building, when async scene building is enabled. In other words, WRBP sends a display list transaction to WR, which gets delegated to the scene builder thread. WRBP then sends a GenerateFrame transaction which can end up getting processed either before or after the scene swap. If the GenerateFrame transaction is processed after the scene swap, then the async scroll offsets in the GenerateFrame transaction are invalidated by the new scene and that results in the wrong scroll offsets. I expect this to be fixed by the changes for bug 1451469, because we'll stop computing the async scroll offsets in WRBP when we send the GenerateFrame transaction, but instead pull them from WR at render time.

[1] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/gfx/layers/wr/WebRenderBridgeParent.cpp#1240

Comment 17

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

Try push from comment 15 is showing debug assertion failures on Windows, which don't make any sense to me. The codepath in question shouldn't be getting run unless the pref is turned on, which it isn't. I'll investigate but it shouldn't block reviewing the patches.

Comment 18

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Created attachment 8965369 [details]
> Bug 1449982 - Introduce pref to control async scene building.
> 
> Until all the pieces are in place, turning on this pref will probably
> break horribly. But we need the pref so we can add the rest of the
> pieces with enabling them by default.

I assume you mean "without enabling them by default".

Comment 19

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

(In reply to Botond Ballo [:botond] from comment #18)
> I assume you mean "without enabling them by default".

Whoops, yes.

Comment 20

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> Try push from comment 15 is showing debug assertion failures on Windows,
> which don't make any sense to me. The codepath in question shouldn't be
> getting run unless the pref is turned on, which it isn't. I'll investigate
> but it shouldn't block reviewing the patches.

https://treeherder.mozilla.org/logviewer.html#?job_id=172152618&repo=try&lineNumber=1776 shows that we're definitely setting the flag somehow but the stack is not symbolicated. I kicked off another try push with more panics/assertions to track down this is happening. From a code inspection it seems impossible, unless maybe gfxPrefs is busted in the GPU process or something.

Comment 21

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> From a code inspection it seems impossible, unless maybe gfxPrefs
> is busted in the GPU process or something.

Fairly sure now this a miscompilation problem. If I store the gfxPrefs::WebRenderAsyncSceneBuild() into a local bool and pass that, it works fine. Passing the pref without putting it in a local var ends up passing true instead of false. Doing one more try push to confirm.

Comment 22

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

Yeah, confirmed. The good scenario [1] has a green try push [2] and the bad scenario [3] has panics [4]. In both cases gfxPrefs::WebRenderAsyncSceneBuild() is false on the C++ side, but in the bad scenario that false gets turned into true crossing the FFI boundary, and is true in rust code.

I'll update the patch to avoid the miscompilation, and see if I can reproduce that locally to debug the generated code.

[1] https://hg.mozilla.org/try/rev/14e0f49dc1e89e37e80886f681aa7de16d955b3d#l4.13
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=14ce3103c4362787ffbea1bfd79d1a1b98bdd07a
[3] https://hg.mozilla.org/try/rev/4e1f0bc7249954d0f455a2d9e9ab969830c91e3f#l4.12
[4] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff5297363729451e19d71b7d03dcff80fdbf0968

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8965366 [details]
Bug 1449982 - Don't hold the sIndirectLayerTreesLock unnecessarily while notifying APZ of a layer tree removal.

https://reviewboard.mozilla.org/r/234090/#review240262

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8965368 [details]
Bug 1449982 - Maintain a map from WrWindowId to APZUpdater.

https://reviewboard.mozilla.org/r/234094/#review240264

::: gfx/layers/apz/public/APZUpdater.h:114
(Diff revision 1)
> +
> +  // Used to manage the mapping from a WR window id to APZUpdater. 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, APZUpdater*> sWindowIdMap;

It's possible to store the WindowId itself as the key type if you specialize std::hash for it (or provide your own hasher as a third template argument to unordered_map).

I'll leave it up to you if you want to do that.

Comment 25

[Botond Ballo [:botond]]

Comment on attachment 8965371 [details]
Bug 1449982 - Implement the WR updater thread registration.

https://reviewboard.mozilla.org/r/234100/#review240268

::: gfx/layers/apz/public/APZUpdater.h:48
(Diff revision 1)
>    explicit APZUpdater(const RefPtr<APZCTreeManager>& aApz);
>  
>    bool HasTreeManager(const RefPtr<APZCTreeManager>& aApz);
>    void SetWebRenderWindowId(const wr::WindowId& aWindowId);
>  
> +  static void SetUpdaterThread(const wr::WrWindowId& aWindowId);

I spent a good minute trying to figure out what this method does based just on its declaration, and wasn't able to.

Please either rename it to something more descriptive (like MarkCurrentThreadAsUpdaterForWindow()), or add a comment.

::: gfx/layers/apz/public/APZUpdater.h:129
(Diff revision 1)
> +  // thread. Additionally, if the async scene building pref is enabled, this
> +  // becomes the thread which will serve as the updater thread.
> +  // This is written to once during init and never cleared, and so reading it
> +  // from multiple threads during normal operation (after initialization)
> +  // without locking should be fine.
> +  Maybe<PlatformThreadId> mUpdaterThreadId;

It looks like this field is only read if async scene building is enabled.

Assuming that's the intention, can we mention that in the comment (and maybe use that fact to simplify the comment, i.e. if that's the case we don't care what this stores unless async scene building is enabled).

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8965372 [details]
Bug 1449982 - Add the task queue for running things on the updater thread.

https://reviewboard.mozilla.org/r/234102/#review240276

::: commit-message-d1eba:5
(Diff revision 1)
> +Bug 1449982 - Add the task queue for running things on the updater thread. r?botond
> +
> +If we're using the WR scene builder thread as the updater thread, we
> +cannot just post a task to its message loop, because it's a rust thread
> +doesn't have a message loop. Instead, we put these tasks into a queue

There's a missing word here (e.g. "it's a rust thread _that_ doesn't have a message loop")

::: gfx/layers/apz/src/APZUpdater.cpp:386
(Diff revision 1)
>  
>  void
>  apz_deregister_updater(mozilla::wr::WrWindowId aWindowId)
>  {
> +  // Run anything that's still left. Note that this function gets called even
> +  // if async scene building is off, but in that case we don't want to do

So this function gets called even if async scene building is off, but apz_run_updater() doesn't?

Comment 27

[Botond Ballo [:botond]]

Comment on attachment 8965374 [details]
Bug 1449982 - Move the WebRenderScrollData storage from WebRenderBridgeParent to APZUpdater.

https://reviewboard.mozilla.org/r/234106/#review240286

::: gfx/layers/apz/public/APZUpdater.h:62
(Diff revision 1)
>    void UpdateHitTestingTree(LayersId aRootLayerTreeId,
>                              Layer* aRoot,
>                              bool aIsFirstPaint,
>                              LayersId aOriginatingLayersId,
>                              uint32_t aPaintSequenceNumber);
>    void UpdateHitTestingTree(LayersId aRootLayerTreeId,

This subtle signature change belies a very significant behavioural change.

This UHTT overload is not like the other any more; please call it something like StoreScrollDataAndUpdateHitTestingTree().

Also, the |aScrollData| parameter has gone from being the root layer tree's scroll data, to being the originating layer tree's scroll data. Please document that, either via a comment or the parameter's name.

::: gfx/layers/apz/src/APZUpdater.cpp:145
(Diff revision 1)
> -                                       bool,
> -                                       LayersId,
> -                                       uint32_t>(
> -      "APZUpdater::UpdateHitTestingTree",
> +    "APZUpdater::UpdateHitTestingTree",
> -      mApz,
> -      func,
> +    [=]() {
> +      self->mScrollData[aOriginatingLayersId] = aScrollData;

WebRenderScrollData is a heavy class to be copying around. Can we pass it by UniquePtr, or at the very least Move() it here?

::: gfx/layers/apz/src/APZUpdater.cpp:151
(Diff revision 1)
> -      aRootLayerTreeId,
> -      aScrollData,
> -      aIsFirstPaint,
> -      aOriginatingLayersId,
> -      aPaintSequenceNumber));
> +      auto root = self->mScrollData.find(aRootLayerTreeId);
> +      if (root == self->mScrollData.end()) {
> +        return;
> +      }
> +      self->mApz->UpdateHitTestingTree(aRootLayerTreeId,
> +          WebRenderScrollDataWrapper(*self.get(), &(root->second)),

RefPtr overloads operator*, so that |*self| is sufficient.

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8965374 [details]
Bug 1449982 - Move the WebRenderScrollData storage from WebRenderBridgeParent to APZUpdater.

https://reviewboard.mozilla.org/r/234106/#review240308

::: gfx/layers/apz/public/APZUpdater.h:125
(Diff revision 1)
> +  // Map from layers id to WebRenderScrollData. This can only be touched on
> +  // the updater thread.
> +  std::unordered_map<LayersId,
> +                     WebRenderScrollData,
> +                     LayersId::HashFn,
> +                     LayersId::EqualFn> mScrollData;

Note: you shouldn't actually need LayersId::EqualFn. Since LayersId overloads operator==, the default equal function should just work.

Comment 29

[Botond Ballo [:botond]]

Comment on attachment 8965374 [details]
Bug 1449982 - Move the WebRenderScrollData storage from WebRenderBridgeParent to APZUpdater.

https://reviewboard.mozilla.org/r/234106/#review240324

::: gfx/layers/wr/WebRenderBridgeParent.cpp:523
(Diff revision 1)
>    if (!rootWrbp) {
>      return;
>    }
>    if (RefPtr<APZUpdater> apz = cbp->GetAPZUpdater()) {
> +    if (aData) {
> -    apz->UpdateFocusState(rootLayersId, GetLayersId(),
> +      apz->UpdateFocusState(rootLayersId, GetLayersId(),

Since we always update the focus state, I think it would make this function's interface cleaner if the signature was:

  UpdateAPZ(const FocusTarget&,
            const Maybe<WebRenderScrollData>&)
            
and the caller passed in data.GetFocusTarget() in the case where the scroll data is also being updated.

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 8965375 [details]
Bug 1449982 - Hook up epoch-based task processing on the updater thread.

https://reviewboard.mozilla.org/r/234108/#review240326

This is just a first pass; I'm posting my comments so far.

I need to think about some of the things in this patch a bit more.

I'd also feel a bit more comfortable if :nical had a look at the change of WebRenderBridgeParent.cpp, and the WR API usage in APZUpdater, as well.

::: gfx/layers/apz/src/APZCTreeManager.cpp:3249
(Diff revision 1)
>  
>  void
> +APZCTreeManager::LockTree()
> +{
> +  AssertOnUpdaterThread();
> +  mTreeLock.Lock();

This really scares me.

How sure are we that there is no way we'll get stuck with the lock held as something in between pre_scene_swap and post_scene_swap running into an error, or a message never arriving, or something?

Do we need a mechanism to ensure the lock is always released, even if something goes wrong (assuming that something isn't going to take down the entire process, in which case it doesn't matter)?

::: gfx/layers/apz/src/APZUpdater.cpp:95
(Diff revision 1)
> +    return;
> +  }
> +
> +  wr::WrPipelineId pipeline;
> +  wr::WrEpoch epoch;
> +  while (wr_pipeline_info_next_removed_pipeline(aInfo, &pipeline)) {

I don't know much about how Rust <--> C++ interop works, but is there an advantage to having an API like this, that crosses the FFI boundary once for every element in a collection (as opposed to querying the entire collection in one call)?

I also don't love this API in that it requires a variable in the enclosing scope to use as the out-param, which makes it seem like the variable is an output of the loop, when it's not.

Scoping the variable might make this clearer:

{
  wr::WrPipelineId pipeline;
  while (wr_pipeline_info_next_removed_pipeline(...)) {
    ...
  }
}

...

{
  wr::WrPipelineId pipeline;
  wr::WrEpoch epoch;
  while (wr_pipeline_info_next_epoch(...)) {
    ...
  }
}

(though an API that just returns a collection would be the cleanest).

::: gfx/layers/apz/src/APZUpdater.cpp:101
(Diff revision 1)
> +    LayersId layersId = wr::AsLayersId(pipeline);
> +    updater->mEpochData.erase(layersId);
> +  }
> +  // Reset the built info for all pipelines, then put it back for the ones
> +  // that got built in this scene swap.
> +  for (auto &i : updater->mEpochData) {

nit: '&' goes next to the type

::: gfx/layers/apz/src/APZUpdater.cpp:104
(Diff revision 1)
> +  // Reset the built info for all pipelines, then put it back for the ones
> +  // that got built in this scene swap.
> +  for (auto &i : updater->mEpochData) {
> +    i.second.mBuilt = Nothing();
> +  }
> +  while (wr_pipeline_info_next_epoch(aInfo, &pipeline, &epoch)) {

It would also be nice to document either these functions, or the WrPipelineInfo struct in bindings.rs. I'd like to see things like:

  - this structure represents information
    about layer trees after a scene build
    [I know "layer tree" is bad term,
    feel free to use a better one]

  - deleted_pipelines contains one entry for
    every layer tree that went away / was
    destroyed since the last scene build
    
  - epochs contains one entry for every
    layer tree that is visible in this scene,
    together with the epoch representing
    the last scene build when it was built
    
  - (there may be layer trees which are
    not destroyed but also not visible, which
    would show up in neither deleted_pipelines
    nor epochs)
    
(I may have gotten some of that wrong, but if so that just underscores the need for documentation.)

::: gfx/layers/apz/src/APZUpdater.cpp:431
(Diff revision 1)
> +bool
> +APZUpdater::IsQueueBlocked() const
> +{
> +  AssertOnUpdaterThread();
> +
> +  for (auto i : mEpochData) {

const auto&

::: gfx/layers/apz/src/APZUpdater.cpp:442
(Diff revision 1)
> +}
> +
> +void
> +APZUpdater::ProcessQueue()
> +{
> +  // Note that running a task might update mEpochData, so we can't

Perhaps add an early exit for if the queue is empty at the beginning of the function?

::: gfx/layers/wr/WebRenderBridgeParent.cpp:509
(Diff revision 1)
>    }
>    return lts->mParent;
>  }
>  
>  void
> -WebRenderBridgeParent::UpdateAPZ(const Maybe<WebRenderScrollData>& aData,
> +WebRenderBridgeParent::UpdateAPZ(const Maybe<wr::Epoch>& aEpoch,

The more Maybe's we add to this function's signature, the less I'm liking it :)

I'm now thinking I'd prefer two methods:

  UpdateAPZFocusState(const FocusTarget&)
  UpdateAPZHitTestTree(const WRScrollData&, const Epoch&)
  
even if it means e.g. calling GetRootCompositorBridgeParent() twice in the case of a nonempty transaction (though I'm willing to be dissuaded on that front).

Comment 31

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #30)
>   UpdateAPZFocusState(const FocusTarget&)
>   UpdateAPZHitTestTree(const WRScrollData&, const Epoch&)
>   
> even if it means e.g. calling GetRootCompositorBridgeParent() twice in the
> case of a nonempty transaction (though I'm willing to be dissuaded on that
> front).

Alternatively, we could have APZUpdater::UpdateHitTestingTree() _also_ call UpdateFocusState(), and then we can have the above two signatures without any redundant code execution (as an added bonus, the two updates could be combined into a single Runnable as well).

Comment 32

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #30)
> The more Maybe's we add to this function's signature, the less I'm liking it
> :)

(In case it's not clear why, it's because having three Maybe parameters makes it seem like there are 8 potential combinations of Nothing/Some values that could be passed, when in fact only 2 of the combinations are valid.)

Comment 33

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #30)
> I need to think about some of the things in this patch a bit more.

Ok, I've thought about it a bit, and my main concern is that this approach can introduce unnecessary latency in certain cases.

Consider a scenario where two layer trees / pipelines are visible, A and B. Suppose B is slow to paint / render, while A is fast.

Now suppose the following happens in the following order (not talking about threads / events yet, just wallclock time):

  1. A completes a display list build.
  2. B completes a display list build.
  3. A responds to a SetTargetAPZC notification.

(SetTargetAPZC is just an example; it can be any message that goes into the queue.)

Now, without WebRender, (1) and (2) only send transactions to the compositor once the correspondings _paints_ complete. Since we're assuming that A is fast to paint while B is slow to paint, let's assume the resulting messages arrive at the compositor in the following order:

  1. A: transaction
  2. A: SetTargetAPZC notification
  3. B: transaction

They are then processed in that order; in particular, APZ can process the SetTargetAPZC notification without waiting for B's slow paint to complete.

Now, let's assume for sake of the example that with WebRender, the slowness of B's paint manifests as scene-building being slow. Since, with WR, we send transactions as soon as the display list build is complete, messages will now (likely) arrive at the compositor in the following order:

  1. A: transaction
  2. B: transaction
  3. A: SetTargetAPZC notification

The messages go into the queue, with the "B: transaction" message enqueuing an epoch requirement that isn't satisfied until the (slow) scene-build for B completes.

This seems unnecessary. If, pre-WR, A's SetTargetAPZC didn't need to wait for B's paint to complete, why does it now need to wait for B's scene-build to complete?

I think this can be addressed by the following modification to the approach:

  - For each message in the queue, track which layers id the message came from.

  - Process the message as soon as _its_ layers id has an up-to-date scene
    (rather than _all_ layers ids having an up-to-date scene).

(This may be equivalent to having a separate queue per layers id; if so, and it's cleaner to implement that way, I'd be fine with that.)

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8965375 [details]
Bug 1449982 - Hook up epoch-based task processing on the updater thread.

Clearing the review flag until the concerns about the tree locking and the added latency are addressed.

Comment 35

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #33)
>   3. A responds to a SetTargetAPZC notification.

(I mean, of course, "A sends a SetTargetAPZC notification in response to an event").

Comment 36

[Nicolas Silva [:nical]]

Comment on attachment 8965367 [details]
Bug 1449982 - Move the window id allocation to CompositorBridgeParent.

https://reviewboard.mozilla.org/r/234092/#review240512

Comment 37

[Nicolas Silva [:nical]]

Comment on attachment 8965369 [details]
Bug 1449982 - Introduce pref to control async scene building.

https://reviewboard.mozilla.org/r/234096/#review240514

Comment 38

[Nicolas Silva [:nical]]

Comment on attachment 8965370 [details]
Bug 1449982 - Add the plumbing for scene builder thread interaction.

https://reviewboard.mozilla.org/r/234098/#review240516

Comment 39

[Nicolas Silva [:nical]]

Comment on attachment 8965373 [details]
Bug 1449982 - Clean up WrEpoch usage.

https://reviewboard.mozilla.org/r/234104/#review240518

Nice.

Comment 40

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

(In reply to Botond Ballo [:botond] from comment #18)
> I assume you mean "without enabling them by default".

Fixed

(In reply to Botond Ballo [:botond] from comment #24)
> It's possible to store the WindowId itself as the key type if you specialize
> std::hash for it (or provide your own hasher as a third template argument to
> unordered_map).
> 
> I'll leave it up to you if you want to do that.

I'll leave this as-is for now.

(In reply to Botond Ballo [:botond] from comment #25)
> > +  static void SetUpdaterThread(const wr::WrWindowId& aWindowId);
> 
> I spent a good minute trying to figure out what this method does based just
> on its declaration, and wasn't able to.
> 
> Please either rename it to something more descriptive (like
> MarkCurrentThreadAsUpdaterForWindow()), or add a comment.

Added a comment to explain this function.

> ::: gfx/layers/apz/public/APZUpdater.h:129
> > +  Maybe<PlatformThreadId> mUpdaterThreadId;
> 
> It looks like this field is only read if async scene building is enabled.
> 
> Assuming that's the intention, can we mention that in the comment (and maybe
> use that fact to simplify the comment, i.e. if that's the case we don't care
> what this stores unless async scene building is enabled).

Yup, updated the comment accordingly.

(In reply to Botond Ballo [:botond] from comment #26)
> > +cannot just post a task to its message loop, because it's a rust thread
> > +doesn't have a message loop. Instead, we put these tasks into a queue
> 
> There's a missing word here (e.g. "it's a rust thread _that_ doesn't have a
> message loop")

Good catch, fixed.

> ::: gfx/layers/apz/src/APZUpdater.cpp:386
> > +  // Run anything that's still left. Note that this function gets called even
> > +  // if async scene building is off, but in that case we don't want to do
> 
> So this function gets called even if async scene building is off, but
> apz_run_updater() doesn't?

Yeah. I added assertions and comments in apz_run_updater (as well as the pre/post swap functions) to that effect.

(In reply to Botond Ballo [:botond] from comment #27)
> >    void UpdateHitTestingTree(LayersId aRootLayerTreeId,
> 
> This subtle signature change belies a very significant behavioural change.
> 
> This UHTT overload is not like the other any more; please call it something
> like StoreScrollDataAndUpdateHitTestingTree().
> 
> Also, the |aScrollData| parameter has gone from being the root layer tree's
> scroll data, to being the originating layer tree's scroll data. Please
> document that, either via a comment or the parameter's name.

I ended up renaming this to UpdateScrollDataAndTreeState, and added some documentation in the .h file. This function now updates the scroll data, focus state, and hit-testing tree all inside one runnable.

> ::: gfx/layers/apz/src/APZUpdater.cpp:145
> > +      self->mScrollData[aOriginatingLayersId] = aScrollData;
> 
> WebRenderScrollData is a heavy class to be copying around. Can we pass it by
> UniquePtr, or at the very least Move() it here?

Done (via Move)

> ::: gfx/layers/apz/src/APZUpdater.cpp:151
> > +      self->mApz->UpdateHitTestingTree(aRootLayerTreeId,
> > +          WebRenderScrollDataWrapper(*self.get(), &(root->second)),
> 
> RefPtr overloads operator*, so that |*self| is sufficient.

Fixed

(In reply to Botond Ballo [:botond] from comment #28)
> > +                     LayersId::EqualFn> mScrollData;
> 
> Note: you shouldn't actually need LayersId::EqualFn. Since LayersId
> overloads operator==, the default equal function should just work.

Fixed. Also filed bug 1452601 to remove it in other places.

(In reply to Botond Ballo [:botond] from comment #29)
> Since we always update the focus state, I think it would make this
> function's interface cleaner if the signature was:
> 
>   UpdateAPZ(const FocusTarget&,
>             const Maybe<WebRenderScrollData>&)

I ended up implementing your other suggestion and split this into two functions, UpdateAPZFocusState and UpdateAPZScrollData. The focus state one just updates the focus state, and the scroll data one calls APZUpdater::UpdateScrollDataAndTreeState which internal updates the focus state and the hit-testing tree. This way the non-empty transaction version still just does the CompositorBridgeParent lookup once, and we have two functions with non-Maybe arguments so it's much clearer.


(In reply to Botond Ballo [:botond] from comment #30)
> > +  AssertOnUpdaterThread();
> > +  mTreeLock.Lock();
> 
> This really scares me.
> 
> How sure are we that there is no way we'll get stuck with the lock held as
> something in between pre_scene_swap and post_scene_swap running into an
> error, or a message never arriving, or something?
> 
> Do we need a mechanism to ensure the lock is always released, even if
> something goes wrong (assuming that something isn't going to take down the
> entire process, in which case it doesn't matter)?

This is a little scary to me as well, but the code that invokes this is really straightforward - it's basically straight-line code without any intermediate conditions, so for now at least I'm confident there's no cases where we would call pre_scene_swap without also calling post_scene_swap except in termination conditions where the scene build thread died before it got a chance to do post_scene_swap, and in that case it doesn't matter. I couldn't think of any other way to do this (i.e. we need to have separate calls to acquire/release the lock) and I'm open to suggestions on ways to make this less scary.

> ::: gfx/layers/apz/src/APZUpdater.cpp:95
> > +  while (wr_pipeline_info_next_removed_pipeline(aInfo, &pipeline)) {
> 
> I don't know much about how Rust <--> C++ interop works, but is there an
> advantage to having an API like this, that crosses the FFI boundary once for
> every element in a collection (as opposed to querying the entire collection
> in one call)?

I agree this isn't great, but I didn't want to fiddle with that API too much in this bug. I think the main advantage is that we don't make a second copy of the data structure, but instead read directly from the one copy held on the rust side. But really this structure isn't that big, so I don't foresee a problem with copying it. I've filed bug 1452620 for this, and I'd like to defer changes here to that bug.

> ::: gfx/layers/apz/src/APZUpdater.cpp:101
> > +  for (auto &i : updater->mEpochData) {
> 
> nit: '&' goes next to the type

Fixed

> ::: gfx/layers/apz/src/APZUpdater.cpp:104
> > +  while (wr_pipeline_info_next_epoch(aInfo, &pipeline, &epoch)) {
> 
> It would also be nice to document either these functions, or the
> WrPipelineInfo struct in bindings.rs. I'd like to see things like:

Will do this in bug 1452620 as well, since I might change the API entirely.

> ::: gfx/layers/apz/src/APZUpdater.cpp:431
> > +  for (auto i : mEpochData) {
> 
> const auto&

Fixed

> ::: gfx/layers/apz/src/APZUpdater.cpp:442
> Perhaps add an early exit for if the queue is empty at the beginning of the
> function?

Done. I wasn't a huge fan of this because now we have two places where we lock the queue inside this function but I don't care that much.

> ::: gfx/layers/wr/WebRenderBridgeParent.cpp:509
> > -WebRenderBridgeParent::UpdateAPZ(const Maybe<WebRenderScrollData>& aData,
> > +WebRenderBridgeParent::UpdateAPZ(const Maybe<wr::Epoch>& aEpoch,
> 
> The more Maybe's we add to this function's signature, the less I'm liking it
> :)
> 
> I'm now thinking I'd prefer two methods:
> 
>   UpdateAPZFocusState(const FocusTarget&)
>   UpdateAPZHitTestTree(const WRScrollData&, const Epoch&)
>   
> even if it means e.g. calling GetRootCompositorBridgeParent() twice in the
> case of a nonempty transaction (though I'm willing to be dissuaded on that
> front).

Yeah I ended up doing something similar to this.

(In reply to Botond Ballo [:botond] from comment #33)
> Ok, I've thought about it a bit, and my main concern is that this approach
> can introduce unnecessary latency in certain cases.

Yeah, thanks for catching this! I kinda had this in the back of mind at some point but I guess I never gave it too much thought. I'll split the queue so that it's per-layers id.

Comment 41

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #40)
> Yeah, thanks for catching this! I kinda had this in the back of mind at some
> point but I guess I never gave it too much thought. I'll split the queue so
> that it's per-layers id.

It turned out to be somewhat complex to actually have multiple std::deque's because I would need to have a map from layers id -> deque (or stuff in the EpochState struct) and clearing out entries from that map then becomes messy. So I just augmented the existing queue to store a (layersid,runnable) and then just processed the runnables with unblocked layers ids.

Comment 42

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

Attachment: Bug 1449982 - Conceptually split the mUpdaterQueue into separate queues per layers id.

This allows us to treat tasks from different layers id as independent,
thereby removing the unnecessary latency increase as described in bug
1449982 comment 33.

Note that we could probably implement this by actually maintaining
separate queues for each layers id, but that involves more overhead
since we would need to have a map from layers id to task queue, and
removing entries from that map becomes tricky with respect to locking
and timing.

Review commit: https://reviewboard.mozilla.org/r/235014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235014/

Comment 43

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

Comment on attachment 8965366 [details]
Bug 1449982 - Don't hold the sIndirectLayerTreesLock unnecessarily while notifying APZ of a layer tree removal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234090/diff/1-2/

Comment 44

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

Comment on attachment 8965367 [details]
Bug 1449982 - Move the window id allocation to CompositorBridgeParent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234092/diff/1-2/

Comment 45

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

Comment on attachment 8965368 [details]
Bug 1449982 - Maintain a map from WrWindowId to APZUpdater.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234094/diff/1-2/

Comment 46

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

Comment on attachment 8965369 [details]
Bug 1449982 - Introduce pref to control async scene building.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234096/diff/1-2/

Comment 47

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

Comment on attachment 8965370 [details]
Bug 1449982 - Add the plumbing for scene builder thread interaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234098/diff/1-2/

Comment 48

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

Comment on attachment 8965371 [details]
Bug 1449982 - Implement the WR updater thread registration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234100/diff/1-2/

Comment 49

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

Comment on attachment 8965372 [details]
Bug 1449982 - Add the task queue for running things on the updater thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234102/diff/1-2/

Comment 50

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

Comment on attachment 8965373 [details]
Bug 1449982 - Clean up WrEpoch usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234104/diff/1-2/

Comment 51

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

Comment on attachment 8965374 [details]
Bug 1449982 - Move the WebRenderScrollData storage from WebRenderBridgeParent to APZUpdater.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234106/diff/1-2/

Comment 52

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

Comment on attachment 8965375 [details]
Bug 1449982 - Hook up epoch-based task processing on the updater thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234108/diff/1-2/

Comment 53

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

(In reply to Botond Ballo [:botond] from comment #30)
> I'd also feel a bit more comfortable if :nical had a look at the change of
> WebRenderBridgeParent.cpp, and the WR API usage in APZUpdater, as well.

@nical: I flagged you for review on another patch for this ^

Comment 54

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #40)
> This is a little scary to me as well, but the code that invokes this is
> really straightforward - it's basically straight-line code without any
> intermediate conditions, so for now at least I'm confident there's no cases
> where we would call pre_scene_swap without also calling post_scene_swap
> except in termination conditions where the scene build thread died before it
> got a chance to do post_scene_swap, and in that case it doesn't matter. I
> couldn't think of any other way to do this (i.e. we need to have separate
> calls to acquire/release the lock) and I'm open to suggestions on ways to
> make this less scary.

The potential concerns I can think of:

  - A catchable panic / exception-like thing escaping the intervening
    code and skipping the unlock.

  - Hanging indefinitely / for a long time inside the intervening
    code (e.g. I notice [1] adds a recv() call).

If we can rule these out with reasonable confidence, then I'm happy with it.

[1] https://github.com/servo/webrender/pull/2613/commits/24eec95667aa036025f4bcf822f12cfd7f0891c3

Comment 55

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

(In reply to Botond Ballo [:botond] from comment #54)
> The potential concerns I can think of:
> 
>   - A catchable panic / exception-like thing escaping the intervening
>     code and skipping the unlock.

All our rust code is built with panic=abort, and rust doesn't have other exceptions as far as I know. So this shouldn't be an issue. In C++ we would probably use an RAII wrapper here, so I wrote a similar thing (untested) for this code, the patch is at https://github.com/staktrace/webrender/commit/f724f2a3849c2ce86eda6e234c61f7e8459c8cd1 but honestly I feel less confident with that code than the original code so unless you feel strongly I'd rather not land it.

>   - Hanging indefinitely / for a long time inside the intervening
>     code (e.g. I notice [1] adds a recv() call).

The only way this can happen is if the render backend thread itself gets wedged, which should again only happen in case of shutdown or deadlock. The recv() call is on a channel that's specific to the message, so there's never anything else on that channel.

Comment 56

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> I'll update the patch to avoid the miscompilation, and see if I can
> reproduce that locally to debug the generated code.

I can't seem to reproduce the problem locally, and just using a local bool var doesn't work either, probably because it gets optimized away (in my try push previously I had a MOZ_ASSERT that was preventing it from getting optimized away, but that's not an assert we want to land). I'll have to try more complicated things.

Comment 57

[Botond Ballo [:botond]]

Comment on attachment 8965375 [details]
Bug 1449982 - Hook up epoch-based task processing on the updater thread.

https://reviewboard.mozilla.org/r/234108/#review240704

::: gfx/layers/apz/public/APZUpdater.h:54
(Diff revisions 1 - 2)
>    void SetWebRenderWindowId(const wr::WindowId& aWindowId);
>  
> +  /**
> +   * This function is invoked from rust on the scene builder thread when it
> +   * is created. It effectively tells the APZUpdater "the current thread is
> +   * the updater thread from this window id" and allows APZUpdater to remember

_for_ this window id?

::: gfx/layers/apz/src/APZUpdater.cpp:211
(Diff revisions 1 - 2)
>        self->mEpochData[aOriginatingLayersId].mRequired = aEpoch;
>      }
>    ));
>    RunOnUpdaterThread(NS_NewRunnableFunction(
>      "APZUpdater::UpdateHitTestingTree",
> -    [=]() {
> +    [=,aScrollData=Move(aScrollData)]() {

The first use of a C++14 init-capture in APZ code. Nice :)

(On the other hand, sigh, I now need to get moving and implement this in Eclipse if I don't want to see a gigantic syntax error here :p)

::: gfx/layers/apz/src/APZUpdater.cpp:446
(Diff revisions 1 - 2)
>  
>  void
>  APZUpdater::ProcessQueue()
>  {
> +  { // scope lock to check for emptiness
> +    MutexAutoLock lock(mQueueLock);

Do we actually need to acquire the lock for the check?

Seems like the worst thing that can happen in case of a race, is that empty() gives the wrong answer. If that answer is "not empty", we proceed to acquire the lock again and get the correct answer.

If the answer is "empty", then the queue must just have been populated by another thread; the current thread could easily have lost the race and correctly early-exited here, so early-exiting doesn't seem like a bad outcome.

::: gfx/layers/wr/WebRenderBridgeParent.cpp:523
(Diff revisions 1 - 2)
> +  }
> +}
> +
> +void
> +WebRenderBridgeParent::UpdateAPZScrollData(const wr::Epoch& aEpoch,
> +                                           const WebRenderScrollData& aData)

While I don't think it makes a difference in what it compiles down to, making this parameter |WebRenderScrollData&&| would be conceptually more correct.

Comment 58

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #57)
> ::: gfx/layers/apz/src/APZUpdater.cpp:446
> (Diff revisions 1 - 2)
> >  
> >  void
> >  APZUpdater::ProcessQueue()
> >  {
> > +  { // scope lock to check for emptiness
> > +    MutexAutoLock lock(mQueueLock);
> 
> Do we actually need to acquire the lock for the check?
> 
> Seems like the worst thing that can happen in case of a race, is that
> empty() gives the wrong answer. If that answer is "not empty", we proceed to
> acquire the lock again and get the correct answer.
> 
> If the answer is "empty", then the queue must just have been populated by
> another thread; the current thread could easily have lost the race and
> correctly early-exited here, so early-exiting doesn't seem like a bad
> outcome.

Then again, we should probably listen to Hans Boehm [1] who argues that no data race in C++ is truly benign, and keep the lock.

[1] https://www.usenix.org/legacy/event/hotpar11/tech/final_files/Boehm.pdf

Comment 59

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

(In reply to Botond Ballo [:botond] from comment #57)
> > +   * is created. It effectively tells the APZUpdater "the current thread is
> > +   * the updater thread from this window id" and allows APZUpdater to remember
> 
> _for_ this window id?

Whoops, fixed.

> ::: gfx/layers/wr/WebRenderBridgeParent.cpp:523
> > +WebRenderBridgeParent::UpdateAPZScrollData(const wr::Epoch& aEpoch,
> > +                                           const WebRenderScrollData& aData)
> 
> While I don't think it makes a difference in what it compiles down to,
> making this parameter |WebRenderScrollData&&| would be conceptually more
> correct.

Ok, will update that. Does the same apply to APZUpdater::UpdateScrollDataAndTreeState?

(In reply to Botond Ballo [:botond] from comment #58)
> 
> Then again, we should probably listen to Hans Boehm [1] who argues that no
> data race in C++ is truly benign, and keep the lock.

Yeah I'm ok with keeping the lock. With the changes in the last patch the early-return seems more worth it.

Comment 60

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #59)
> > While I don't think it makes a difference in what it compiles down to,
> > making this parameter |WebRenderScrollData&&| would be conceptually more
> > correct.
> 
> Ok, will update that. Does the same apply to
> APZUpdater::UpdateScrollDataAndTreeState?

Yeah, we should make it WebRenderScrollata&& throughout the call chain.

Comment 61

[Botond Ballo [:botond]]

Comment on attachment 8966263 [details]
Bug 1449982 - Conceptually split the mUpdaterQueue into separate queues per layers id.

https://reviewboard.mozilla.org/r/235014/#review240752

::: gfx/layers/apz/src/APZCTreeManager.cpp:1981
(Diff revision 1)
>      // nsBaseWidget (as opposed to over PAPZCTreeManager). We want this function
>      // to run on the updater thread, so bounce it over.
>      MOZ_ASSERT(XRE_IsParentProcess());
>  
>      GetUpdater()->RunOnUpdaterThread(
> +        mRootLayersId,

Why not aGuid.mLayersId?

::: gfx/layers/apz/src/APZUpdater.cpp:452
(Diff revision 1)
> +        // still blocked back in and finish
> +        std::swap(mUpdaterQueue, blockedTasks);
>          break;
>        }
>        task = mUpdaterQueue.front();
>        mUpdaterQueue.pop_front();

I think the correctness of this (specifically, the property that it preserves relative ordering of events within a layers id) depends on the blocked state of a layers id not changing (from blocked to unblocked) in the middle of a ProcessQueue() operation.

(Otherwise, if a new event for a previously blocked layers id is enqueued at the right time, it can "jump the queue" in front of events waiting in blockedTasks.)

Do we know that this is the case (a layers id will not become unblocked in the middle of a ProcessQueue)? (I think the answer is yes, because only CompleteSceneSwap() will unblock a layers id, and that can only get called on the updater thread and therefore not in the middle of ProcessQueue().)

Even if it is the case, it would be good to document the fact that we are relying on this property.

Comment 62

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

(In reply to Botond Ballo [:botond] from comment #61)
> >      GetUpdater()->RunOnUpdaterThread(
> > +        mRootLayersId,
> 
> Why not aGuid.mLayersId?

They should always be the same here, I can add an assertion to that effect if you prefer. If they do ever differ, mRootLayersId would be the safer option because it comes from a more trustworthy source (CompositorBridgeParent) than aGuid.mLayersId (which comes from widget but maybe in the future from less-trustworthy call sites).

> ::: gfx/layers/apz/src/APZUpdater.cpp:452
> I think the correctness of this (specifically, the property that it
> preserves relative ordering of events within a layers id) depends on the
> blocked state of a layers id not changing (from blocked to unblocked) in the
> middle of a ProcessQueue() operation.

Good point, I hadn't actually considered this but I agree with your analysis. I'll add a comment to this effect.

Comment 63

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

Comment on attachment 8965366 [details]
Bug 1449982 - Don't hold the sIndirectLayerTreesLock unnecessarily while notifying APZ of a layer tree removal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234090/diff/2-3/

Comment 64

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

Comment on attachment 8965367 [details]
Bug 1449982 - Move the window id allocation to CompositorBridgeParent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234092/diff/2-3/

Comment 65

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

Comment on attachment 8965368 [details]
Bug 1449982 - Maintain a map from WrWindowId to APZUpdater.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234094/diff/2-3/

Comment 66

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

Comment on attachment 8965369 [details]
Bug 1449982 - Introduce pref to control async scene building.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234096/diff/2-3/

Comment 67

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

Comment on attachment 8965370 [details]
Bug 1449982 - Add the plumbing for scene builder thread interaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234098/diff/2-3/

Comment 68

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

Comment on attachment 8965371 [details]
Bug 1449982 - Implement the WR updater thread registration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234100/diff/2-3/

Comment 69

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

Comment on attachment 8965372 [details]
Bug 1449982 - Add the task queue for running things on the updater thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234102/diff/2-3/

Comment 70

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

Comment on attachment 8965373 [details]
Bug 1449982 - Clean up WrEpoch usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234104/diff/2-3/

Comment 71

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

Comment on attachment 8965374 [details]
Bug 1449982 - Move the WebRenderScrollData storage from WebRenderBridgeParent to APZUpdater.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234106/diff/2-3/

Comment 72

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

Comment on attachment 8965375 [details]
Bug 1449982 - Hook up epoch-based task processing on the updater thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234108/diff/2-3/

Comment 73

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

Comment on attachment 8966263 [details]
Bug 1449982 - Conceptually split the mUpdaterQueue into separate queues per layers id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235014/diff/1-2/

Comment 74

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #62)
> (In reply to Botond Ballo [:botond] from comment #61)
> > >      GetUpdater()->RunOnUpdaterThread(
> > > +        mRootLayersId,
> > 
> > Why not aGuid.mLayersId?
> 
> They should always be the same here, I can add an assertion to that effect
> if you prefer. If they do ever differ, mRootLayersId would be the safer
> option because it comes from a more trustworthy source
> (CompositorBridgeParent) than aGuid.mLayersId (which comes from widget but
> maybe in the future from less-trustworthy call sites).

What about the case where this comes from a child process [1]?

[1] https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/dom/ipc/TabChild.cpp#544

Comment 75

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #74)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #62)
> > (In reply to Botond Ballo [:botond] from comment #61)
> > > >      GetUpdater()->RunOnUpdaterThread(
> > > > +        mRootLayersId,
> > > 
> > > Why not aGuid.mLayersId?
> > 
> > They should always be the same here, I can add an assertion to that effect
> > if you prefer. If they do ever differ, mRootLayersId would be the safer
> > option because it comes from a more trustworthy source
> > (CompositorBridgeParent) than aGuid.mLayersId (which comes from widget but
> > maybe in the future from less-trustworthy call sites).
> 
> What about the case where this comes from a child process [1]?
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/dom/ipc/TabChild.cpp#544

Ok, I see now that there is a comment in this branch of UpdateZoomConstraints() that says:

 // This can happen if we're in the UI process and got a call directly from
 // nsBaseWidget (as opposed to over PAPZCTreeManager). We want this function
 // to run on the updater thread, so bounce it over.

However, is this comment still accurate with the updater thread refactor in place? As far as I can tell, APZCTreeManagerParent still lives on the compositor thread, which can now be different from the updated thread.

Comment 76

[Botond Ballo [:botond]]

Comment on attachment 8966263 [details]
Bug 1449982 - Conceptually split the mUpdaterQueue into separate queues per layers id.

https://reviewboard.mozilla.org/r/235014/#review240788

Anyways, r+ assuming the UpdateZoomConstraints issue is sorted out.

Comment 77

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

(In reply to Botond Ballo [:botond] from comment #75)
> Ok, I see now that there is a comment in this branch of
> UpdateZoomConstraints() that says:
> 
>  // This can happen if we're in the UI process and got a call directly from
>  // nsBaseWidget (as opposed to over PAPZCTreeManager). We want this function
>  // to run on the updater thread, so bounce it over.
> 
> However, is this comment still accurate with the updater thread refactor in
> place? As far as I can tell, APZCTreeManagerParent still lives on the
> compositor thread, which can now be different from the updated thread.

Shoot, you're right, I was thinking the comment was still accurate but it's not (in the case where async scene-building is enabled). It didn't show up in my try pushes because I never did one with async scene building enabled (and maybe it's not covered by any tests anyway, who knows).

I'll use aGuid.mLayersId here, and I'll fix up the comment/other assertion in another bug since it's conceptually different from this bug.

Comment 78

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

Comment on attachment 8965366 [details]
Bug 1449982 - Don't hold the sIndirectLayerTreesLock unnecessarily while notifying APZ of a layer tree removal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234090/diff/3-4/

Comment 79

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

Comment on attachment 8965367 [details]
Bug 1449982 - Move the window id allocation to CompositorBridgeParent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234092/diff/3-4/

Comment 80

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

Comment on attachment 8965368 [details]
Bug 1449982 - Maintain a map from WrWindowId to APZUpdater.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234094/diff/3-4/

Comment 81

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

Comment on attachment 8965369 [details]
Bug 1449982 - Introduce pref to control async scene building.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234096/diff/3-4/

Comment 82

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

Comment on attachment 8965370 [details]
Bug 1449982 - Add the plumbing for scene builder thread interaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234098/diff/3-4/

Comment 83

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

Comment on attachment 8965371 [details]
Bug 1449982 - Implement the WR updater thread registration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234100/diff/3-4/

Comment 84

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

Comment on attachment 8965372 [details]
Bug 1449982 - Add the task queue for running things on the updater thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234102/diff/3-4/

Comment 85

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

Comment on attachment 8965373 [details]
Bug 1449982 - Clean up WrEpoch usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234104/diff/3-4/

Comment 86

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

Comment on attachment 8965374 [details]
Bug 1449982 - Move the WebRenderScrollData storage from WebRenderBridgeParent to APZUpdater.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234106/diff/3-4/

Comment 87

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

Comment on attachment 8965375 [details]
Bug 1449982 - Hook up epoch-based task processing on the updater thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234108/diff/3-4/

Comment 88

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

Comment on attachment 8966263 [details]
Bug 1449982 - Conceptually split the mUpdaterQueue into separate queues per layers id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235014/diff/2-3/

Comment 89

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

The latest patch set has all the above comments addressed. I filed bug 1452845 to fix APZCTreeManager::UpdateZoomConstraints (and anything else that turns out to be buggy).

Try push with the above patchset is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6f8a5ca70446eb0185301d63b9b0fb55e2a3c57

Same patches rebased on master are at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f2b25aece7d4233876538d4320e93d3ad196cfc

Note also that in order to avoid the miscompilation issue I ended up adding an if condition with separate branches for the enabled and disabled case, that's in the WebRenderAPI.cpp change in part 4.

Comment 90

[Nicolas Silva [:nical]]

Comment on attachment 8965375 [details]
Bug 1449982 - Hook up epoch-based task processing on the updater thread.

https://reviewboard.mozilla.org/r/234108/#review241050

Comment 91

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

Comment on attachment 8965366 [details]
Bug 1449982 - Don't hold the sIndirectLayerTreesLock unnecessarily while notifying APZ of a layer tree removal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234090/diff/4-5/

Comment 92

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

Comment on attachment 8965367 [details]
Bug 1449982 - Move the window id allocation to CompositorBridgeParent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234092/diff/4-5/

Comment 93

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

Comment on attachment 8965368 [details]
Bug 1449982 - Maintain a map from WrWindowId to APZUpdater.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234094/diff/4-5/

Comment 94

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

Comment on attachment 8965369 [details]
Bug 1449982 - Introduce pref to control async scene building.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234096/diff/4-5/

Comment 95

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

Comment on attachment 8965370 [details]
Bug 1449982 - Add the plumbing for scene builder thread interaction.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234098/diff/4-5/

Comment 96

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

Comment on attachment 8965371 [details]
Bug 1449982 - Implement the WR updater thread registration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234100/diff/4-5/

Comment 97

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

Comment on attachment 8965372 [details]
Bug 1449982 - Add the task queue for running things on the updater thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234102/diff/4-5/

Comment 98

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

Comment on attachment 8965373 [details]
Bug 1449982 - Clean up WrEpoch usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234104/diff/4-5/

Comment 99

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

Comment on attachment 8965374 [details]
Bug 1449982 - Move the WebRenderScrollData storage from WebRenderBridgeParent to APZUpdater.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234106/diff/4-5/

Comment 100

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

Comment on attachment 8965375 [details]
Bug 1449982 - Hook up epoch-based task processing on the updater thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234108/diff/4-5/

Comment 101

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

Comment on attachment 8966263 [details]
Bug 1449982 - Conceptually split the mUpdaterQueue into separate queues per layers id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235014/diff/3-4/

Comment 102

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/433668f7db7b
Don't hold the sIndirectLayerTreesLock unnecessarily while notifying APZ of a layer tree removal. r=botond
https://hg.mozilla.org/integration/autoland/rev/7af325560dfa
Move the window id allocation to CompositorBridgeParent. r=nical
https://hg.mozilla.org/integration/autoland/rev/c40aa91bc0b6
Maintain a map from WrWindowId to APZUpdater. r=botond
https://hg.mozilla.org/integration/autoland/rev/7b83f46b5fb3
Introduce pref to control async scene building. r=nical
https://hg.mozilla.org/integration/autoland/rev/f14bce5630f5
Add the plumbing for scene builder thread interaction. r=nical
https://hg.mozilla.org/integration/autoland/rev/1c9e68c30a81
Implement the WR updater thread registration. r=botond
https://hg.mozilla.org/integration/autoland/rev/0c1ab4c3acd4
Add the task queue for running things on the updater thread. r=botond
https://hg.mozilla.org/integration/autoland/rev/3eab97d46f5a
Clean up WrEpoch usage. r=nical
https://hg.mozilla.org/integration/autoland/rev/526faf046875
Move the WebRenderScrollData storage from WebRenderBridgeParent to APZUpdater. r=botond
https://hg.mozilla.org/integration/autoland/rev/1a6398d3f9b3
Hook up epoch-based task processing on the updater thread. r=botond,nical
https://hg.mozilla.org/integration/autoland/rev/3542146e9973
Conceptually split the mUpdaterQueue into separate queues per layers id. r=botond

Comment 103

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/433668f7db7b
https://hg.mozilla.org/mozilla-central/rev/7af325560dfa
https://hg.mozilla.org/mozilla-central/rev/c40aa91bc0b6
https://hg.mozilla.org/mozilla-central/rev/7b83f46b5fb3
https://hg.mozilla.org/mozilla-central/rev/f14bce5630f5
https://hg.mozilla.org/mozilla-central/rev/1c9e68c30a81
https://hg.mozilla.org/mozilla-central/rev/0c1ab4c3acd4
https://hg.mozilla.org/mozilla-central/rev/3eab97d46f5a
https://hg.mozilla.org/mozilla-central/rev/526faf046875
https://hg.mozilla.org/mozilla-central/rev/1a6398d3f9b3
https://hg.mozilla.org/mozilla-central/rev/3542146e9973