Closed Bug 1329968 Opened 8 years ago Closed 5 months ago

Conversation on web.whatsapp.com is not initially given display port

Categories

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

53 Branch
defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: jnicol, Assigned: jnicol)

References

()

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(1 file)

Open web.whatsapp.com and click on conversation in the list on the left hand side so that a conversation is opened on the right. The contents of the conversation should have a displayport and be on a scrollable layer so that scrolling through it performs well. But in practice this is not the case until the user actually starts to scroll, which causes a relayerisation and lots of repainting, meaning the initial scroll is not smooth. This is because nsLayoutUtils::MaybeCreateDisplayPort() has already given the conversation list a displayport, so skips the conversation contents. We don't want to give every frame a display port, but certainly in this case they should both get one. So we should give every frame a display port up until we reach a limit, based on area. This can be implemented very similarly to the "AGR Budget" in nsDisplayListBuilder.
If you can get this working I have no objections. I thought that in the past this was suggested at some point and there turned out to be a circular dependency of some sort - we needed to know which elements had a displayport at some point before a budgeting system would apply, and changing the set of elements afterwards would require a relayout. Not sure if I'm remembering correctly though.
Keywords: perf
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Whiteboard: [gfx-noted]
Version: unspecified → 53 Branch
Note that another approach would be to ask them to use "will-change: scroll-position" on the conversation view, to force layerization - assuming that still works. Or maybe we could drop MaybeCreateDisplayPort and just assume will-change:scroll-position on everything until we run out of budget.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1) > I thought that in the past > this was suggested at some point and there turned out to be a circular > dependency of some sort Ah, this is what I was thinking of: https://bugzilla.mozilla.org/show_bug.cgi?id=1103106#c22
I think that would be the case if we were to do anything clever, but a simple first-come first-served budget should be fine. From a quick skim of the bug you just linked to that seems to be confirmed (I've only ever known will-change budget to be like this). Asking them to add will-change should still work but I'm assuming this isn't the only site of its kind. And I think we want to keep this as a separate budget, so that we don't prevent frames that are actually marked as will-change from being treated as such?
Attachment #8834374 - Flags: review?(bugmail)
Comment on attachment 8834374 [details] Bug 1329968 - Allow multiple async-scrollable frames to be given displayports. https://reviewboard.mozilla.org/r/110322/#review111562 Overall this looks sane, although I have a few concerns below. ::: layout/base/nsLayoutUtils.cpp:3326 (Diff revision 1) > + // Only create the display port if we fall within the budget. > + bool inBudget = aBuilder.AddToDisplayPortBudget(aScrollFrame); > + if (inBudget) { > - // If we don't already have a displayport, calculate and set one. > + // If we don't already have a displayport, calculate and set one. > - if (!haveDisplayPort) { > + if (!haveDisplayPort) { I think you probably want to invert the order of the two conditions here. That is, if haveDisplayPort is already true, then you don't want to be calling AddToDisplayPortBudget because it will eat up some of the budget for a thing that has already been assigned a displayport for other reasons. Although maybe that's intentional? If so then the budget should probably be larger. ::: layout/painting/nsDisplayList.h:1494 (Diff revision 1) > + // Set of frames already counted in display port budget > + nsTHashtable<nsPtrHashKey<nsIFrame> > mDisplayPortBudgetSet; I'm not sure if it's safe to keep nsIFrame pointers around like this (they might become invalid if the frame is recreated, but I'm not sure if that can happen during the displaylist builder lifetime). tnikkel should probably look at this. Also you should be able to get rid of the space between the two '>' characters, according to https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code ::: layout/painting/nsDisplayList.cpp:1723 (Diff revision 1) > } > return onBudget; > } > > + > +const float gDisplayPortBudgetAreaMultiplier = 1.0; I'd prefer to make this a live pref. ::: layout/painting/nsDisplayList.cpp:1738 (Diff revision 1) > + const uint32_t budgetLimit = gDisplayPortBudgetAreaMultiplier * > + nsPresContext::AppUnitsToIntCSSPixels(area.width) * > + nsPresContext::AppUnitsToIntCSSPixels(area.height); > + > + const uint32_t cost = GetLayerizationCost(aFrame->GetSize()); > + const bool onBudget = mUsedDisplayPortBudget + cost <= budgetLimit; nit: i know adding parentheses on this expression is not necessary for correctness but I always feel it's clearer parenthesized.
Comment on attachment 8834374 [details] Bug 1329968 - Allow multiple async-scrollable frames to be given displayports. Adding :tnikkel as well.
Attachment #8834374 - Flags: review?(tnikkel)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) > ::: layout/base/nsLayoutUtils.cpp:3326 > (Diff revision 1) > > + // Only create the display port if we fall within the budget. > > + bool inBudget = aBuilder.AddToDisplayPortBudget(aScrollFrame); > > + if (inBudget) { > > - // If we don't already have a displayport, calculate and set one. > > + // If we don't already have a displayport, calculate and set one. > > - if (!haveDisplayPort) { > > + if (!haveDisplayPort) { > > I think you probably want to invert the order of the two conditions here. > That is, if haveDisplayPort is already true, then you don't want to be > calling AddToDisplayPortBudget because it will eat up some of the budget for > a thing that has already been assigned a displayport for other reasons. > Although maybe that's intentional? If so then the budget should probably be > larger. Wouldn't that mean on every paint we keep adding a new displayport to every element that could get one (because the budget is never used by anything that has a displayport already, and those displayports may have come from this function on a previous paint)? > ::: layout/painting/nsDisplayList.h:1494 > (Diff revision 1) > > + // Set of frames already counted in display port budget > > + nsTHashtable<nsPtrHashKey<nsIFrame> > mDisplayPortBudgetSet; > > I'm not sure if it's safe to keep nsIFrame pointers around like this (they > might become invalid if the frame is recreated, but I'm not sure if that can > happen during the displaylist builder lifetime). tnikkel should probably > look at this. Also you should be able to get rid of the space between the > two '>' characters, according to > https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code We can't do frame construction during painting. We have assertions to check that. Display items etc hold frame pointers.
Comment on attachment 8834374 [details] Bug 1329968 - Allow multiple async-scrollable frames to be given displayports. https://reviewboard.mozilla.org/r/110322/#review111904 ::: layout/painting/nsDisplayList.cpp:1728 (Diff revision 1) > +const float gDisplayPortBudgetAreaMultiplier = 1.0; > + > +bool > +nsDisplayListBuilder::AddToDisplayPortBudget(nsIFrame* aFrame) > +{ > + if (mDisplayPortBudgetSet.Contains(aFrame)) { Do we actually need to check for duplicate calls? Seems like we should get one call per frame per paint. If not, maybe we should fix that. ::: layout/painting/nsDisplayList.cpp:1732 (Diff revision 1) > +{ > + if (mDisplayPortBudgetSet.Contains(aFrame)) { > + return true; > + } > + > + const nsRect area = aFrame->PresContext()->GetVisibleArea(); We need to use the visible area of root prescontext. The frame could be in a child prescontext with arbitrary size. ::: layout/painting/nsDisplayList.cpp:1737 (Diff revision 1) > + const nsRect area = aFrame->PresContext()->GetVisibleArea(); > + const uint32_t budgetLimit = gDisplayPortBudgetAreaMultiplier * > + nsPresContext::AppUnitsToIntCSSPixels(area.width) * > + nsPresContext::AppUnitsToIntCSSPixels(area.height); > + > + const uint32_t cost = GetLayerizationCost(aFrame->GetSize()); I think we have to use 64bit ints to do this computation because the size of a frame could exceed 65k x 65k. This code is already used by will-change, so is there something I'm missing that makes this not a problem?
Attachment #8834374 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #9) > Wouldn't that mean on every paint we keep adding a new displayport to every > element that could get one (because the budget is never used by anything > that has a displayport already, and those displayports may have come from > this function on a previous paint)? Oh, that's true, good point.
Comment on attachment 8834374 [details] Bug 1329968 - Allow multiple async-scrollable frames to be given displayports. Dropping review flag until other comments above are addressed.
Attachment #8834374 - Flags: review?(bugmail)
Comment on attachment 8834374 [details] Bug 1329968 - Allow multiple async-scrollable frames to be given displayports. https://reviewboard.mozilla.org/r/110322/#review111980 ::: layout/base/nsLayoutUtils.cpp:3326 (Diff revision 1) > + // Only create the display port if we fall within the budget. > + bool inBudget = aBuilder.AddToDisplayPortBudget(aScrollFrame); > + if (inBudget) { > - // If we don't already have a displayport, calculate and set one. > + // If we don't already have a displayport, calculate and set one. > - if (!haveDisplayPort) { > + if (!haveDisplayPort) { Tim explained on bugzilla the reason why they're this way around. The previous code still called aBuilder.SetHaveScrollableDisplayPort() if a displayport had already been assigned for other reasons. I'm not 100% what those other reasons could be, but I think we want them counted. The budget limit is of course tweakable but I thought that 1 x the pres context area would mean the same behaviour on the common case. ::: layout/painting/nsDisplayList.cpp:1728 (Diff revision 1) > +const float gDisplayPortBudgetAreaMultiplier = 1.0; > + > +bool > +nsDisplayListBuilder::AddToDisplayPortBudget(nsIFrame* aFrame) > +{ > + if (mDisplayPortBudgetSet.Contains(aFrame)) { No we don't. We do for will-change, and I just copied it. ::: layout/painting/nsDisplayList.cpp:1732 (Diff revision 1) > +{ > + if (mDisplayPortBudgetSet.Contains(aFrame)) { > + return true; > + } > + > + const nsRect area = aFrame->PresContext()->GetVisibleArea(); Okay. If we use the root prescontext then using "height: 100vh;" in the test_layerization to use up the budget no longer works. (It does locally where the pres context is almost the root prescontext size, but on try they are very different.) Once I make the area multiplier a pref I can just set that to zero, however. ::: layout/painting/nsDisplayList.cpp:1737 (Diff revision 1) > + const nsRect area = aFrame->PresContext()->GetVisibleArea(); > + const uint32_t budgetLimit = gDisplayPortBudgetAreaMultiplier * > + nsPresContext::AppUnitsToIntCSSPixels(area.width) * > + nsPresContext::AppUnitsToIntCSSPixels(area.height); > + > + const uint32_t cost = GetLayerizationCost(aFrame->GetSize()); I'm not aware of the original reasoning for using a uint32. I'll happily change them all to use 64 bits.
(In reply to Jamie Nicol [:jnicol] from comment #13) > Okay. If we use the root prescontext then using "height: 100vh;" in the > test_layerization to use up the budget no longer works. (It does locally > where the pres context is almost the root prescontext size, but on try they > are very different.) On Try individual tests are loaded into an iframe rather than into the toplevel window. If it helps, you can replicate this behaviour locally by running an entire directory of tests rather than just one test. (And if the other tests take too long to run, you can disable them temporarily in the directory's mochitest.ini file.)
Comment on attachment 8834374 [details] Bug 1329968 - Allow multiple async-scrollable frames to be given displayports. https://reviewboard.mozilla.org/r/110322/#review112138 r+ with comments addressed, and assuming the test passes on all platforms. It's probably worth doing a try push for it if you haven't already. ::: layout/painting/nsDisplayList.cpp:1656 (Diff revision 2) > // There's significant overhead for each layer created from Gecko > // (IPC+Shared Objects) and from the backend (like an OpenGL texture). > // Therefore we set a minimum cost threshold of a 64x64 area. > int minBudgetCost = 64 * 64; > > - uint32_t budgetCost = > + uint64_t budgetCost = I think for the 64-bit-ness of this to actually take effect, at least one of the multiplicands needs to be casted to 64-bit. Otherwise the multiplication happens in 32-bit space (potentially overflowing) and *then* gets put into a 64-bit value. So you need: uint64_t budgetCost = std::max(minBudgetCost, (uint64_t)nsPresContext::AppUnitsToIntCSSPixels(aSize.width) * nsPresContext::AppUnitsToIntCSSPixels(aSize.height)); Same goes for the other 64-bit things. ::: layout/painting/nsDisplayList.cpp:1729 (Diff revision 2) > + const nsRect area = presContext->GetVisibleArea(); > + const uint64_t budgetLimit = gfxPrefs::LayoutDisplayPortBudgetMultiplier() * > + nsPresContext::AppUnitsToIntCSSPixels(area.width) * > + nsPresContext::AppUnitsToIntCSSPixels(area.height); Instead of doing this computation for every scrollable frame, can we instead invert it? So instead of having mUsedDisplayPortBudget which increases, we can have a mAvailableDisplayPortBudget which gets initialized to the (budgetMultiplier * area.width * area.height), and then we subtract from that in AddToDisplayPortBudget. It should reduce the overhead of this function which potentially could get called a lot. The nsDisplayListBuilder is constructed with a reference from which you can get the root pres context to compute the initial budget.
Attachment #8834374 - Flags: review?(bugmail) → review+

Layout/base/nsLayoutUtils.cpp:3326
(Diff revision 1)

  • // Only create the display port if we fall within the budget.
  • bool inBudget = aBuilder.AddToDisplayPortBudget(aScrollFrame);
  • if (inBudget) {
  • // If we don't already have a displayport, calculate and set one.
  •  // If we don't already have a displayport, calculate and set one.
    
  • if (!haveDisplayPort) {
  •  if (!haveDisplayPort) {
    

Tim explained on bugzilla the reason why they're this way around.

The previous code still called aBuilder.SetHaveScrollableDisplayPort() if a displayport had already been assigned for other reasons. I'm not 100% what those other reasons could be, but I think we want them counted.

The budget limit is of course tweakable but I thought that 1 x the pres context area would mean the same behaviour on the common case.

::: layout/painting/nsDisplayList.cpp:1728

https://techcloud7.org/blog/try-cool-whatsapp-statuses-love-quotes/

Severity: normal → S3

Lots of things have changed since this bug was filed. I'm not sure about whether the underlying display port code has changed, but importantly the symptom described by the bug is fixed by webrender - on first opening a conversation now give the scrollable content its own picture cache slice, meaning we scroll without invalidation.

Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: