Closed Bug 1453463 Opened 6 years ago Closed 6 years ago

Save a map from guid -> APZC in the APZCTreeManager

Categories

(Core :: Panning and Zooming, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files)

As previously discussed on bug 1391318 comment 13 and 14, we need a way to sample APZ transforms without acquiring the APZ tree lock. One of the things we want to do in order to make that possible is to keep a map of APZCs in the tree manager with its own lock. As a side benefit, this map can also be used to simplify some existing code.
Comment on attachment 8967156 [details]
Bug 1453463 - Refactor to make unordered_maps with guid keys a little cleaner.

https://reviewboard.mozilla.org/r/235812/#review242356
Attachment #8967156 - Flags: review?(botond) → review+
Comment on attachment 8967157 [details]
Bug 1453463 - Keep an APZC map on APZCTreeManager.

https://reviewboard.mozilla.org/r/235814/#review242358

::: gfx/layers/apz/src/APZCTreeManager.h:742
(Diff revision 2)
> +   * acquire the tree lock. mMapLock must be acquired while accessing or
> +   * modifying mApzcMap.
> +   */
> +  mutable mozilla::Mutex mMapLock;
> +  std::unordered_map<ScrollableLayerGuid,
> +                     AsyncPanZoomController*,

Can we keep RefPtrs in this map? I don't think the pointers in the map can outlive the APZC instances they point to as the code currently stands, but using RefPtrs would safeguard against future modifications where that may not be the case.

::: gfx/layers/apz/src/APZCTreeManager.h:744
(Diff revision 2)
> +   */
> +  mutable mozilla::Mutex mMapLock;
> +  std::unordered_map<ScrollableLayerGuid,
> +                     AsyncPanZoomController*,
> +                     ScrollableLayerGuid::HashIgnoringPresShellFn,
> +                     ScrollableLayerGuid::EqualIgnoringPresShellFn> mApzcMap;

Why does it make sense to care about the pres shell id in mZoomConstraints, but not in this map?
Attachment #8967157 - Flags: review?(botond)
Comment on attachment 8967158 [details]
Bug 1453463 - Update existing code to be more efficient by using the new map.

https://reviewboard.mozilla.org/r/235816/#review242360

::: gfx/layers/apz/src/APZCTreeManager.cpp:2516
(Diff revision 2)
>  
>      // Guard against a possible infinite-loop condition. If we hit this, the
>      // layout code that generates the handoff parents did something wrong.
>      MOZ_ASSERT(apzc->GetScrollHandoffParentId() != apzc->GetGuid().mScrollId);
> -
> -    // Find the AsyncPanZoomController instance with a matching layersId and
> +    RefPtr<AsyncPanZoomController> scrollParent =
> +        GetTargetAPZC(apzc->GetGuid().mLayersId, apzc->GetScrollHandoffParentId());

Nice simplification :)
Attachment #8967158 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #9)
> Can we keep RefPtrs in this map? I don't think the pointers in the map can
> outlive the APZC instances they point to as the code currently stands, but
> using RefPtrs would safeguard against future modifications where that may
> not be the case.

Sure, will change this.

> ::: gfx/layers/apz/src/APZCTreeManager.h:744
> Why does it make sense to care about the pres shell id in mZoomConstraints,
> but not in this map?

So I'm actually not sure it ever makes sense to care about the pres shell id any more. It was introduced back when we still had things like ROOT_SCROLL_ID such that the (layers id, scroll id) tuple wasn't unique enough. It's quite possible that since we got rid of the ROOT_SCROLL_ID we don't need to have the presshell id as part of the guid any more. If we look at the places ScrollableLayerGuid::mPresShellId is actually used [1] there doesn't seem to be anything useful there. In APZCTreeManager::SetupScrollbarDrag we use it in the AsyncDragMetrics constructor, but then AsyncDragMetrics::mPresShellId is only ever used to pass it back to ScrollableLayerGuid instances. I can't find any if condition that is conditional upon the presShell id. I'm happy to file a follow-up to remove this entirely.

[1] https://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_mozilla%3A%3Alayers%3A%3AScrollableLayerGuid%3E_1&redirect=false
[2] https://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_mozilla%3A%3Alayers%3A%3AAsyncDragMetrics%3E_1&redirect=false
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> If we look
> at the places ScrollableLayerGuid::mPresShellId is actually used [1] there
> doesn't seem to be anything useful there. [...] I can't find any if condition
> that is conditional upon the presShell id. I'm happy to file a follow-up to 
> remove this entirely.

The presence of mPresShellId in ScrollableLayerGuid's operator== and hash function makes any ScrollableLayerGuid comparison or lookup with a ScrollableLayerGuid key a codepath that's potentially conditional on the pres shell id.

For example, during tree building we compare guids to determine whether to reuse an APZC from the previous tree [1].

So suppose in one layer transaction, you have a scrollable layer S, and in the next you have a scrollable layer whose view id is the same as S but its pres shell id is different. Right now, we construct a new APZC for it, but if we removed mPresShellId, we would instead reuse the APZC.

Now, I don't know if that scenario (having a pres shell id change while the view id stays the same) is actually possible on the layout side. If not, then I agree that the press hell id is useless and we should remove it.

[1] https://searchfox.org/mozilla-central/rev/1b6599866513f853bedcb461369631294605f625/gfx/layers/apz/src/APZCTreeManager.cpp#922
(In reply to Botond Ballo [:botond] from comment #12)
> Now, I don't know if that scenario (having a pres shell id change while the
> view id stays the same) is actually possible on the layout side. If not,
> then I agree that the press hell id is useless and we should remove it.

A follow-up thought on this: as far as mApzcMap is concerned, that's used to associate layers sharing an APZC in the *same* transaction, so ignoring pres shell ids there won't make a difference unless we can have two guids that differ only by pres shell id in the same transaction. As long as we can rule that out, I'm happy with that change.
(In reply to Botond Ballo [:botond] from comment #12)
> The presence of mPresShellId in ScrollableLayerGuid's operator== and hash
> function makes any ScrollableLayerGuid comparison or lookup with a
> ScrollableLayerGuid key a codepath that's potentially conditional on the
> pres shell id.
> 
> For example, during tree building we compare guids to determine whether to
> reuse an APZC from the previous tree [1].

True

> So suppose in one layer transaction, you have a scrollable layer S, and in
> the next you have a scrollable layer whose view id is the same as S but its
> pres shell id is different. Right now, we construct a new APZC for it, but
> if we removed mPresShellId, we would instead reuse the APZC.
> 
> Now, I don't know if that scenario (having a pres shell id change while the
> view id stays the same) is actually possible on the layout side. If not,
> then I agree that the press hell id is useless and we should remove it.

I think conceptually it is possible, because the view id is a property on the DOM tree (it's stored on nsIContent) while the presshell id is a function of the presshell, and I think it's possible for multiple presshells to render the same DOM tree. However in practice I don't think this actually happens. I wrote the patch to strip out the presShellId from various places and it's green on try [1] although I don't know if that means much. I also used the try build to test print preview on windows (which IMO is the most likely thing to create a new presshell on an existing document) and that seems to work fine as well. That being said I'm still not convinced that this change is safe, and don't really want to land it at this point - maybe we can revisit it in the future.

(In reply to Botond Ballo [:botond] from comment #13)
> A follow-up thought on this: as far as mApzcMap is concerned, that's used to
> associate layers sharing an APZC in the *same* transaction, so ignoring pres
> shell ids there won't make a difference unless we can have two guids that
> differ only by pres shell id in the same transaction. As long as we can rule
> that out, I'm happy with that change.

This is a good point. I'm fairly sure we can rule this out, because the transaction comes from a layer manager which is associated with a single presshell (except for iframes inside the document, but you wouldn't have the same scrollid on elements in a document as well as in an iframe inside the document).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=800c8e62cbaa83497b53293b35680c5625b5c217
Comment on attachment 8967157 [details]
Bug 1453463 - Keep an APZC map on APZCTreeManager.

https://reviewboard.mozilla.org/r/235814/#review242726
Attachment #8967157 - Flags: review?(botond) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21ad02243968
Refactor to make unordered_maps with guid keys a little cleaner. r=botond
https://hg.mozilla.org/integration/autoland/rev/6e9808db792e
Keep an APZC map on APZCTreeManager. r=botond
https://hg.mozilla.org/integration/autoland/rev/9a18481b44cd
Update existing code to be more efficient by using the new map. r=botond
You need to log in before you can comment on or make changes to this bug.