For content we allocate buffers bigger than the actually visible portion to faciliate smooth scrolling. For chrome content like floating thebes elements or scroll boxes, such as the awesomepage url list or the settings page in fennec, we are very strict about what gets painted. This leads to many paint/resize/blit operations, which are expensive for openGL backends. If we really want to leverage the gpu and provide smooth scrolling we need to pre-cache out of view chrome contents to a reasonable extent.

Adding some people for discussion.

We could adapt setDisplayportForElement so that Fennec could specify how much to retain. It shouldn't be very hard--we should just remove the metadata from nsDisplayScrollLayer for the chrome process and I think everything else might just work.

Comment #2 would be fine, but we could also make chrome scrollboxes automatically pre-cache.

So, honestly I'd much rather fix painting being slow than start caching things. Memory is precious for mobile devices. From the callgraph, it's not as if uploading to GL is any sort of bottleneck here. We spend a lot of time...painting backgrounds it seems?

So I'm figuring stuff out... first: nsDisplayScrollLayer and nsDisplayScrollInfoLayer rely on the scrolllayer count property on their mScrolled frame, right? it is increased for every item generated and reduced for every item that gets optimized away. But it turns out it is only decremented for the TryMerge case, not the ShouldFlattenAway case - so you can end up with a flat list of display items and the count is still > 0 - is this intended? secondly, about the fennec use case. we use the gfxScrollFrame for Preferences scrolling. It turns out that the background items and the text/foreground items are separated by the scrollframes and the images above the settings scroller (the settings tab headers if you will). I can reason about the scrollbars somehow - they are an intrinsic part of the scroller, but why the images? they don't even overlap the scroller. With the separated bg and front items their respective clips don't get merged and in turn the scrolllayers don't get merged, so we end up with two thebes layers that get independently painted.

(In reply to comment #6) > So I'm figuring stuff out... > first: nsDisplayScrollLayer and nsDisplayScrollInfoLayer rely on the > scrolllayer count property on their mScrolled frame, right? it is increased > for every item generated and reduced for every item that gets optimized away. > But it turns out it is only decremented for the TryMerge case, not the > ShouldFlattenAway case - so you can end up with a flat list of display items > and the count is still > 0 - is this intended? Yes. If we get to ShouldFlattenAway, *all* the remaining nsDisplayScrollLayers for that frame should disappear. > secondly, about the fennec use case. > we use the gfxScrollFrame for Preferences scrolling. It turns out that the > background items and the text/foreground items are separated by the > scrollframes and the images above the settings scroller (the settings tab > headers if you will). I can reason about the scrollbars somehow - they are > an intrinsic part of the scroller, but why the images? they don't even > overlap the scroller. > With the separated bg and front items their respective clips don't get > merged and in turn the scrolllayers don't get merged, so we end up with two > thebes layers that get independently painted. I'm not getting a clear picture of the problem with this description. Maybe you could post the relevant display list here (or actually, in a new bug)? The more succinct, the easier it will be to understand, so if you can please hide as much in the preferences as possible that gets your case across. It certainly seems like this may be a case where there's no reason we can't generate a new layer, but I think we'll need a reduced example to tease out the proper answer. In short, it sounds like you are on the right track, but the display list needs a little massaging.

I was able to prevent the images from getting in between the scrolled content's border&background and text by forcing a new stacking context: <box id="panel-controls" class="panel-row-header" oncommand="BrowserUI.switchPane(event.target.getAttribute('linkedpanel'));" style="position:relative; z-index:0;"> here: http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser.xul#381 as for the scrollbars, I'm looking at the AppendScrollPartsTo function to see if we can force them into a different stacking context or at least put them at the bottom of the context they are in right now. Also I'm looking at why the scrolled content is divided into two clips with backgrounds and texts at all - shouldn't they be in a single clip?

display lists after I managed to force the images into a different stacking context, but the scrollbars still intervene. We create two thebes layers the size of the viewport and lock/paint/unlock both of them with every scrolled pixel.

So the changes I propose - sorry my tree is full of printfs right now - are: 1.: Prevent the toolbar tabs from interfering with the scrollboxes below: - <box id="panel-controls" class="panel-row-header" oncommand="BrowserUI.switchPane(event.target.getAttribute('linkedpanel'));"> + <box id="panel-controls" class="panel-row-header" oncommand="BrowserUI.switchPane(event.target.getAttribute('linkedpanel'));" style="position:relative; z-index:0;"> 2.: Set display ports on relevant scrollboxes: let winCwu = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils); winCwu.setDisplayPortForElement(0, 0, 854, 2048, document.getElementById("prefs-list")._scrollbox); 3.: We want a flag here, or want to enable it always if we have a display port set: PRBool createLayersForScrollbars = mIsRoot && mOuter->PresContext()->IsRootContentDocument(); createLayersForScrollbars=true; //FIXME 4.: I think we want to reduce the count of nsDisplayScrollLayers on frames if we merge them: PRBool nsDisplayScrollLayer::TryMerge(nsDisplayListBuilder* aBuilder, nsDisplayItem* aItem) { if (aItem->GetType() != TYPE_SCROLL_LAYER) { return PR_FALSE; } nsDisplayScrollLayer* other = static_cast<nsDisplayScrollLayer*>(aItem); if (other->mScrolledFrame != this->mScrolledFrame) { return PR_FALSE; } + FrameProperties props = mScrolledFrame->Properties(); + int count=GetScrollLayerCount() - 1; + props.Set(nsIFrame::ScrollLayerCount(), + reinterpret_cast<void*>(count)); mList.AppendToBottom(&other->mList); return PR_TRUE; } 5.: Get the scrollbars out between the text and border&background of the scrolled content: nsGfxScrollFrameInner::AppendScrollPartsTo(nsDisplayListBuilder* aBuilder, const nsRect& aDirtyRect, const nsDisplayListSet& aLists, const nsDisplayListCollection& aDest, PRBool& aCreateLayer) { nsresult rv = NS_OK; PRBool hasResizer = HasResizer(); for (nsIFrame* kid = mOuter->GetFirstChild(nsnull); kid; kid = kid->GetNextSibling()) { if (kid != mScrolledFrame) { if (kid == mResizerBox && hasResizer) { continue; } rv = mOuter->BuildDisplayListForChild(aBuilder, kid, aDirtyRect, aDest, nsIFrame::DISPLAY_CHILD_FORCE_STACKING_CONTEXT); NS_ENSURE_SUCCESS(rv, rv); // DISPLAY_CHILD_FORCE_STACKING_CONTEXT put everything into the // PositionedDescendants list. - ::AppendToTop(aBuilder, aLists.BorderBackground(), + ::AppendToTop(aBuilder, aLists.PositionedDescendants(), aDest.PositionedDescendants(), kid, aCreateLayer); } } return rv; } all in all this gives us ZERO locking/painting/unlocking on the scroller surface for scrolling after the settings dialog has been finished loading. I still do get one paint event for the scrollbar every frame, though. The resulting texture image is a measly 917px high for the settings page, I consider that nothing compared to content pages. And it should only live as long as the page is open anyways. PLUS due to the toolbar images and scrollbars spliting the scrolled content's display lists, they couldn't merge and we had two viewport sized RGBA buffers allocated, while now we have a single one.

Good work! 1, 2 and 3 sound reasonable. 4 already exists on trunk, not sure which rev you're looking at. For 5, I would use the Content() list instead. > I still do get one paint event for the scrollbar every frame, though. Isn't that because we have to repaint the scrollbar as the thumb moves? (In reply to comment #8) > Also I'm looking at why the scrolled content is divided into two clips with > backgrounds and texts at all - shouldn't they be in a single clip? That's because in CSS, the backgrounds of elements outside a overflow:hidden element can be drawn between the background of the overflow:hidden element and its contents.

Here's a visual draw-log from what I have with the forthcoming patches and the patch from bug 524925. Where we had redraws for the scrollbars for every scroll movement we now only have very few paints and the entailing locks. Also visible is the settings page drawn in a single texture that is bigger than the visible screen (852x480). Scrolling is smooth as butter.

Here's all changes in one patch so far. I still need to separate this into individual changesets.

Comment on attachment 552406 [details] [diff] [review] needs to be separated A little drive by feedback, but good job removing so much painting! :D > PRBool > nsDisplayScrollLayer::ShouldFlattenAway(nsDisplayListBuilder* aBuilder) > { >- return GetScrollLayerCount() > 1; >+ int count=GetScrollLayerCount(); >+ if(count > 1) { >+ if(mScrolledFrame) { >+ FrameProperties props = mScrolledFrame->Properties(); >+ props.Set(nsIFrame::ScrollLayerCount(), >+ reinterpret_cast<void*>(count-1)); >+ } >+ return PR_TRUE; >+ } >+ return PR_FALSE; > } What is this change doing? If scroll layers aren't next to each other then we probably shouldn't make a scroll layer. I haven't tried this patch, but it has to NOT generate a scroll layer in cases like this: http://people.mozilla.org/~bstover/zindex.html > NS_IMETHODIMP > nsSliderFrame::HandleEvent(nsPresContext* aPresContext, I don't know what these are. Does Fennec use these?

Comment on attachment 552406 [details] [diff] [review] needs to be separated Review of attachment 552406 [details] [diff] [review]: ----------------------------------------------------------------- Put the gfxASurface changes in a separate patch. ::: layout/base/nsDisplayList.cpp @@ +1933,5 @@ > + FrameProperties props = mScrolledFrame->Properties(); > + props.Set(nsIFrame::ScrollLayerCount(), > + reinterpret_cast<void*>(count-1)); > + } > + return PR_TRUE; This shouldn't be needed. ::: layout/generic/nsGfxScrollFrame.cpp @@ +1876,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > // DISPLAY_CHILD_FORCE_STACKING_CONTEXT put everything into the > // PositionedDescendants list. > + ::AppendToTop(aBuilder, aLists.PositionedDescendants(), //aLists->aDest > + aDest.PositionedDescendants(), kid, //aDest->aSource? What are these comments about? I suggested using the Content() destination list, not the PositionedDescendants() list. @@ +1993,5 @@ > // Since making new layers is expensive, only use nsDisplayScrollLayer > // if the area is scrollable. > nsRect scrollRange = GetScrollRange(); > ScrollbarStyles styles = GetScrollbarStylesFromFrame(); > + mShouldBuildLayer = ( We need some kind of new check here ... I don't think we want to do this everywhere yet. ::: layout/xul/base/src/nsScrollbarFrame.cpp @@ -75,5 @@ > - // We want to be a reflow root since we use reflows to move the > - // slider. Any reflow inside the scrollbar frame will be a reflow to > - // move the slider and will thus not change anything outside of the > - // scrollbar or change the size of the scrollbar frame. > - mState |= NS_FRAME_REFLOW_ROOT; Why are you removing this? ::: layout/xul/base/src/nsSliderFrame.cpp @@ +738,5 @@ > + transform.AppendInt(newThumbRect.y/60); > + transform.AppendLiteral("px)"); > + nsAutoString prop; > + prop.AppendLiteral("-moz-transform"); > + result = cssDecl->SetProperty(prop,transform, EmptyString()); Ugh, this is horrible! :-) What we probably should do is extend nsDisplayTransform and nsIFrame::IsTransformed() to check for an "extra transform" frame property, and include that in the transform. With a state bit that is set when the property is available.

added display port to awesomebar panels and the other scroll lists next to preferences. removed unnecessary changes, but some of roc's issues remain. There's two issues I noticed in very long scrollboxes: When the history page lazy-loads items to the end of the list, there's a short black flicker. If you scroll way down and then way back up, you get a white area. otherwise scrolling is noticably smoother.

about the new transform property: can we use this call from here http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#7671 to tell the frame to cache the transform from the display style into it's local mTransform, setting the MAY_BE_TRANSFORMED flag and then just returning mTransForm from GetTransformMatrix?

No. I would use a new state bit to indicate whether the frame has an "extra transform". When there's an extra transform, store it as a frame property.

The patch grew considerably due to the new property and I had to shuffle a few functions around that depended on only css ever transforming a frame.

Comment on attachment 554397 [details] [diff] [review] WIP - Added AdditionalTransform frame property Review of attachment 554397 [details] [diff] [review]: ----------------------------------------------------------------- The rest basically looks OK although I don't think you should move that code around to become methods of nsIFrame. ::: layout/generic/nsIFrame.h @@ +2743,5 @@ > nsRect mRect; > nsIContent* mContent; > nsStyleContext* mStyleContext; > nsIFrame* mParent; > + gfx3DMatrix mAdditionalTransform; You can't do this, this bloats the size of frames massively. Use a frame property via NS_DECLARE_FRAME_PROPERTY etc.

Comment on attachment 555370 [details] [diff] [review] WIP - using frame property Review of attachment 555370 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsDisplayList.cpp @@ +2428,5 @@ > + > + gfx3DMatrix result; > + > + if (aFrame->GetStateBits() & NS_FRAME_ADDITIONAL_TRANSFORM) { > + gfx3DMatrix* trafo = static_cast<gfx3DMatrix*>( call it "transform", "trafo" is quite unclear :-) @@ +2430,5 @@ > + > + if (aFrame->GetStateBits() & NS_FRAME_ADDITIONAL_TRANSFORM) { > + gfx3DMatrix* trafo = static_cast<gfx3DMatrix*>( > + Properties().Get(AdditionalTransform())); > + if(trafo) Assert that trafo/transform is non-null, since it should be if the state bit is set. @@ -2442,5 @@ > nsDisplayTransform::GetFrameBoundsForTransform(aFrame)); > > /* Get the matrix, then change its basis to factor in the origin. */ > PRBool dummy; > - gfx3DMatrix result = How can removing this line be correct? This wouldn't even compile since 'result' is used. @@ +2747,5 @@ > const nsPoint &aOrigin, > const nsRect* aBoundsOverride) > { > NS_PRECONDITION(aFrame, "Can't take the transform based on a null frame!"); > + NS_PRECONDITION(aFrame->IsTransformed(), IsTransformed is not the same as "GetStyleDisplay()->HasTransform() || flags & NS_FRAME_ADDITIONAL_TRANSFORM". For example, nsSVGOuterSVGFrame overrides it. You need a new method, something like HasSpecifiedTransform(). ::: layout/generic/nsFrame.cpp @@ +4194,5 @@ > + } else { > + mState |= NS_FRAME_ADDITIONAL_TRANSFORM; > + gfx3DMatrix* trafo = new gfx3DMatrix; > + *trafo = aTransform; > + Properties().Set(AdditionalTransform(), trafo); You can just pass "new gfx3DMatrix(aTransform)" here. ::: layout/generic/nsGfxScrollFrame.cpp @@ +1999,1 @@ > (styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN || This makes us build nsDisplayScrollLayers on desktop products. I don't think we want to do that, it's overhead we don't need or use on desktop Firefox at the moment. I'm not sure the best way to control this though. For chrome, it really should be enabled on a per-element basis somehow, since it's pointless if chrome isn't going to set up displayports for it. ::: layout/xul/base/src/nsSliderFrame.cpp @@ +720,5 @@ > + thumbFrame->SetAdditionalTransform( > + gfx3DMatrix::Translation( > + NSAppUnitsToFloatPixels(newThumbRect.x, scaleFactor), > + NSAppUnitsToFloatPixels(newThumbRect.y, scaleFactor), > + 0)); You still need to invalidate here.

> How can removing this line be correct? This wouldn't even compile since 'result' is used. I pulled the declaration of result up, check the first block you quoted :) > You still need to invalidate here. Can you explain why? Is there a case where we influence either the scrolled content or the thumb through transforming it?

Changing the layer tree doesn't itself cause repainting. You need to invalidate to ensure that the scrollbar thumb area is repainted.

I played around with displayport some more and found a way to improve the sidebar panning experience and making bug 608983 obsolete with a much simpler patch. I notice some superfluous drawing of the url bar, still.

Comment on attachment 556543 [details] [diff] [review] Version using css2 transform again Review of attachment 556543 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +1871,5 @@ > + // PositionedDescendants list. We also put everything into our PositionedDescendants > + // list so it comes after all the scrolled content and doesn't interfere in merging the > + // nsDisplayScrollItems generated by the scrolled content. > + ::AppendToTop(aBuilder, aLists.PositionedDescendants(), // the destination > + aDest.PositionedDescendants(), kid, // the source I still think the destination list should be the Content() list. @@ +1993,2 @@ > (styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN || > styles.mVertical != NS_STYLE_OVERFLOW_HIDDEN) && Like I said before, this starts creating scrollinfo layers for all scrollable areas in desktop Firefox, and that's probably not what we want. You should use a pref or something to control this. ::: layout/xul/base/src/nsSliderFrame.cpp @@ -433,5 @@ > - > - // set the thumb's coord to be the current pos * the ratio. > - nsRect thumbRect(clientRect.x, clientRect.y, thumbSize.width, thumbSize.height); > - PRInt32& thumbPos = (IsHorizontal() ? thumbRect.x : thumbRect.y); > - thumbPos += NSToCoordRound(pos * mRatio); Are you just removing support for the "reverse" attribute? Why is that OK? @@ +669,5 @@ > + public: > + nsSliderThumbTranslator(nsIFrame* aThumbFrame, nsPoint aTargetTranslation) > + : mThumbFrame(aThumbFrame) > + , mTargetTranslation(aTargetTranslation) {}; > + NS_IMETHOD Run() You shouldn't hold onto mThumbFrame. That frame could be destroyed before this runs. Instead, hold onto the content node with a strong pointer (nsCOMPtr<nsIContent>). @@ +745,2 @@ > > + thumbFrame->InvalidateTransformLayer(); This should be done by nsSliderThumbTranslator.

Your display ports are very large. Is it necessary to make them that large?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28) > > + ::AppendToTop(aBuilder, aLists.PositionedDescendants(), // the destination > > + aDest.PositionedDescendants(), kid, // the source > > I still think the destination list should be the Content() list. I double checked and it turns out with the css transform we can use the Content list. I had some weird case before where using anything but the positioned desendants list would interfere in layer building. > > @@ +1993,2 @@ > > (styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN || > > styles.mVertical != NS_STYLE_OVERFLOW_HIDDEN) && > > Like I said before, this starts creating scrollinfo layers for all > scrollable areas in desktop Firefox, and that's probably not what we want. > You should use a pref or something to control this. A global preference, as in about:config? Or a per-frame setting? > > ::: layout/xul/base/src/nsSliderFrame.cpp > @@ -433,5 @@ > > - > > - // set the thumb's coord to be the current pos * the ratio. > > - nsRect thumbRect(clientRect.x, clientRect.y, thumbSize.width, thumbSize.height); > > - PRInt32& thumbPos = (IsHorizontal() ? thumbRect.x : thumbRect.y); > > - thumbPos += NSToCoordRound(pos * mRatio); > > Are you just removing support for the "reverse" attribute? Why is that OK? Because identical code lives further down in nsSliderFrame::CurrentPositionChanged, where the positioning happens exclusively now. Here in DoLayout we only resize the thumb, which does not depend on reverse or not. We can probably slim this function down further. > > @@ +669,5 @@ > > + public: > > + nsSliderThumbTranslator(nsIFrame* aThumbFrame, nsPoint aTargetTranslation) > > + : mThumbFrame(aThumbFrame) > > + , mTargetTranslation(aTargetTranslation) {}; > > + NS_IMETHOD Run() > > You shouldn't hold onto mThumbFrame. That frame could be destroyed before > this runs. Instead, hold onto the content node with a strong pointer > (nsCOMPtr<nsIContent>). > > @@ +745,2 @@ > > > > + thumbFrame->InvalidateTransformLayer(); > > This should be done by nsSliderThumbTranslator. Can we do this if we only hold a reference to the content, not the frame?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29) > Your display ports are very large. Is it necessary to make them that large? They are the size of the maximum texture size on mobile. The actual textures that get created are only the size of what is actually there in terms of content, though. I can provide you with a log containing all the generated images.

(In reply to Florian Hänel [:heeen] from comment #30) > > @@ +1993,2 @@ > > > (styles.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN || > > > styles.mVertical != NS_STYLE_OVERFLOW_HIDDEN) && > > > > Like I said before, this starts creating scrollinfo layers for all > > scrollable areas in desktop Firefox, and that's probably not what we want. > > You should use a pref or something to control this. > > A global preference, as in about:config? Or a per-frame setting? The former I guess. > > ::: layout/xul/base/src/nsSliderFrame.cpp > > @@ -433,5 @@ > > > - > > > - // set the thumb's coord to be the current pos * the ratio. > > > - nsRect thumbRect(clientRect.x, clientRect.y, thumbSize.width, thumbSize.height); > > > - PRInt32& thumbPos = (IsHorizontal() ? thumbRect.x : thumbRect.y); > > > - thumbPos += NSToCoordRound(pos * mRatio); > > > > Are you just removing support for the "reverse" attribute? Why is that OK? > > Because identical code lives further down in > nsSliderFrame::CurrentPositionChanged, where the positioning happens > exclusively now. Here in DoLayout we only resize the thumb, which does not > depend on reverse or not. We can probably slim this function down further. But we have to reposition the thumb when the slider size changes, right? What ensures that happens, with your patch? > > @@ +745,2 @@ > > > > > > + thumbFrame->InvalidateTransformLayer(); > > > > This should be done by nsSliderThumbTranslator. > Can we do this if we only hold a reference to the content, not the frame? Yes, you can always get the frame for the content by calling GetPrimaryFrame() ... if there is one; it might be null.

hmmm the thumb is positioned at the wrong position after we append items to one of the dynamically growing listboxes (e.g. history). With a translation of 0,0 when we scroll back to the top, it is still somewhere at 1/3 of the screen...

So I think AppendScrollPartsTo() should not change to put the items in Content() or PositionedDescendants(). Instead, we should make the particular list a parameter to AppendScrollPartsTo(). In the !mScrollbarsCanOverlapContent case, we can continue appending to aLists.BorderBackground(). In the mScrollbarsCanOverlapContent, we can append to PositionedDescendants().

I tested the displayport for the sidebars some more and got some weird results. Sometimes stuff typed into the url bar would just not show and my draw log would show it trying to paint a white layer the size of the screen - but nothing shows there - really weird.

Comment on attachment 557500 [details] [diff] [review] latest revision Review of attachment 557500 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: layout/generic/nsGfxScrollFrame.cpp @@ +1468,5 @@ > presContext->LookAndFeel()-> > GetMetric(nsILookAndFeel::eMetric_ScrollbarsCanOverlapContent, canOverlap); > mScrollbarsCanOverlapContent = canOverlap; > mScrollingActive = IsAlwaysActive(); > + sXULScrollLayersEnabled=Preferences::GetBool("layers.acceleration.xul-scrolling", PR_FALSE); Not a layers pref really ... layout.scrolling.layers_always?

Comment on attachment 558247 [details] [diff] [review] latest revision Review of attachment 558247 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +1468,5 @@ > presContext->LookAndFeel()-> > GetMetric(nsILookAndFeel::eMetric_ScrollbarsCanOverlapContent, canOverlap); > mScrollbarsCanOverlapContent = canOverlap; > mScrollingActive = IsAlwaysActive(); > + sXULScrollLayersEnabled=Preferences::GetBool("layout.scrolling.layers-always", PR_FALSE); Use GetCachedBoolPref, I think @@ +1989,5 @@ > // if the area is scrollable. > nsRect scrollRange = GetScrollRange(); > ScrollbarStyles styles = GetScrollbarStylesFromFrame(); > mShouldBuildLayer = > + ((sXULScrollLayersEnabled || XRE_GetProcessType() == GeckoProcessType_Content) && We don't need the process type check here anymore, right? ::: layout/xul/base/src/nsSliderFrame.cpp @@ +373,5 @@ > + mThumbContent = aThumbFrame->GetContent(); > + }; > + NS_IMETHOD Run() > + { > + printf("translating thumb! %i,%i\n", mTargetTranslation.x, mTargetTranslation.y); This needs to be removed @@ +488,5 @@ > + PRInt32 scaleFactor = PresContext()->AppUnitsPerDevPixel(); > + nsContentUtils::AddScriptRunner(new nsSliderThumbTranslator( > + thumbFrame, > + nsPoint(NSAppUnitsToFloatPixels(thumbRect.x, scaleFactor), > + NSAppUnitsToFloatPixels(thumbRect.y, scaleFactor)))); We can't land this until bug 524925 is landed, otherwise we'll be triggering lots of reflows. Have you checked that this works properly on other platforms and themes? I wonder whether this works properly with themes that change the status of scrollbar buttons depending on where the thumb is. This might need to be special-cased for Fennec scrollbars only. ::: mobile/chrome/content/bindings.xml @@ +215,5 @@ > + <![CDATA[ > + // need to handle autocomplete popup differently from batch box > + // also lazy loaded so can't set from browser-ui.js init() like prefs > + let winCwu = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils); > + winCwu.setDisplayPortForElement(0, 0, 2048, 2048, this._items); Please separate out these displayport changes. Someone else should review them.

not sure who to pick, I think stechz has helped me understanding displayport stuff better iirc.

Can you reply to comment #40?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40) > Comment on attachment 558247 [details] [diff] [review] > latest revision > > Review of attachment 558247 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/generic/nsGfxScrollFrame.cpp > @@ +1468,5 @@ > > presContext->LookAndFeel()-> > > GetMetric(nsILookAndFeel::eMetric_ScrollbarsCanOverlapContent, canOverlap); > > mScrollbarsCanOverlapContent = canOverlap; > > mScrollingActive = IsAlwaysActive(); > > + sXULScrollLayersEnabled=Preferences::GetBool("layout.scrolling.layers-always", PR_FALSE); > > Use GetCachedBoolPref, I think the only references to this I find for some PresContext settings, are you sure this is the one you mean? > > @@ +1989,5 @@ > > // if the area is scrollable. > > nsRect scrollRange = GetScrollRange(); > > ScrollbarStyles styles = GetScrollbarStylesFromFrame(); > > mShouldBuildLayer = > > + ((sXULScrollLayersEnabled || XRE_GetProcessType() == GeckoProcessType_Content) && > > We don't need the process type check here anymore, right? we used to do this for remote unconditionally before my change, are you sure you want to make this conditional on the pref setting now for remoat and xul? > > @@ +488,5 @@ > > + PRInt32 scaleFactor = PresContext()->AppUnitsPerDevPixel(); > > + nsContentUtils::AddScriptRunner(new nsSliderThumbTranslator( > > + thumbFrame, > > + nsPoint(NSAppUnitsToFloatPixels(thumbRect.x, scaleFactor), > > + NSAppUnitsToFloatPixels(thumbRect.y, scaleFactor)))); > > We can't land this until bug 524925 is landed, otherwise we'll be triggering > lots of reflows. > > Have you checked that this works properly on other platforms and themes? > > I wonder whether this works properly with themes that change the status of > scrollbar buttons depending on where the thumb is. > > This might need to be special-cased for Fennec scrollbars only. Sorry, I don't have enough platforms at hand to thoroughly test this everywhere. I opted to ifdef it for GFX_OPTIMIZE_MOBILE instead and asked :cwiiis to test on android. > > ::: mobile/chrome/content/bindings.xml > @@ +215,5 @@ > > + <![CDATA[ > > + // need to handle autocomplete popup differently from batch box > > + // also lazy loaded so can't set from browser-ui.js init() like prefs > > + let winCwu = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils); > > + winCwu.setDisplayPortForElement(0, 0, 2048, 2048, this._items); > > Please separate out these displayport changes. Someone else should review > them. as stechz probably won't be up for review I'll have to find someone else. I think bjacob tutored stechz on displayport stuff?

(In reply to Florian Hänel [:heeen] from comment #44) > the only references to this I find for some PresContext settings, are you > sure this is the one you mean? Sorry, I meant Preferences::AddBoolVarCache. > > @@ +1989,5 @@ > > > // if the area is scrollable. > > > nsRect scrollRange = GetScrollRange(); > > > ScrollbarStyles styles = GetScrollbarStylesFromFrame(); > > > mShouldBuildLayer = > > > + ((sXULScrollLayersEnabled || XRE_GetProcessType() == GeckoProcessType_Content) && > > > > We don't need the process type check here anymore, right? > > we used to do this for remote unconditionally before my change, are you sure > you want to make this conditional on the pref setting now for remoat and xul? OK, you're right. > Sorry, I don't have enough platforms at hand to thoroughly test this > everywhere. I opted to ifdef it for GFX_OPTIMIZE_MOBILE instead and asked > :cwiiis to test on android. OK. > > ::: mobile/chrome/content/bindings.xml > > @@ +215,5 @@ > > > + <![CDATA[ > > > + // need to handle autocomplete popup differently from batch box > > > + // also lazy loaded so can't set from browser-ui.js init() like prefs > > > + let winCwu = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils); > > > + winCwu.setDisplayPortForElement(0, 0, 2048, 2048, this._items); > > > > Please separate out these displayport changes. Someone else should review > > them. > > as stechz probably won't be up for review I'll have to find someone else. I > think bjacob tutored stechz on displayport stuff? Don't think so. Stechz or cjones.

Comment on attachment 558427 [details] [diff] [review] the xul/fennec-side displayport stuff I not a good reviewer of fennec frontend code.

It is my understanding that :stechz has left mozilla and is not up for review.

I am happy to still do reviews, though I am still not convinced about this approach! Looking at the scrolling FPS for local things on my phone, I am rather certain something else is wrong. Uploading time might be important when we are almost at 60FPS (see https://bugzilla.mozilla.org/show_bug.cgi?id=670930#c11--we should be able to upload tons of pixels for a handful of milliseconds), but I think I am seeing something more like 5FPS. I'd say this is really just putting a band-aid over another problem we really need to fix. I'm happy to re-examine once we figure out the terribad problem, but until then I'm not willing to r+ this.

without these patches I'm seeing 5-6 FPS max in preferences view, and with these patches 20FPS. Upload costs are different from device to device, also texture create/destroy is very different, depends on drivers implementation. Without cached scrollable view we have both problems which makes scrolling just horrible... This is not band-aid, because we definitely should do textures churning and uploading as less as possible, and I don't see other way then having it pre-cached (btw we do the same for content viewport)

Here profile report for chrome pref scrolling, with these patches... I see we still do lot of calculations in layout, but that also happening for software rendering...

Comment on attachment 558427 [details] [diff] [review] the xul/fennec-side displayport stuff The code looks good to me. Should we also do this for the tab list, which is scrollable in tablet mode? What is the impact of this change on RAM/memory footprint?

(In reply to Matt Brubeck (:mbrubeck) from comment #52) > What is the impact of this change on RAM/memory footprint? I just tested TextureImage creation and destruction and images are destroyed as soon as the related content isn't visible anymore, eg.: You open the preferences panel and in my case a Texture of 854x1068 pixels is created where normally there would be a 854x400px texture. As soon as I return to the main view, the texture is destroyed. So any increase in vram is only temporary as long as it is required.

(In reply to Benjamin Stover (:stechz) from comment #49) > I am happy to still do reviews, though I am still not convinced about this > approach! Looking at the scrolling FPS for local things on my phone, I am > rather certain something else is wrong. Uploading time might be important > when we are almost at 60FPS (see > https://bugzilla.mozilla.org/show_bug.cgi?id=670930#c11--we should be able > to upload tons of pixels for a handful of milliseconds), but I think I am > seeing something more like 5FPS. I'd say this is really just putting a > band-aid over another problem we really need to fix. > > I'm happy to re-examine once we figure out the terribad problem, but until > then I'm not willing to r+ this. This patch saves not only uploading, but also re-flowing due to a moving scrollbar and painting due to scrolled content as well as the scrollbar needing to be painted onto the content. I can't rule out that there still lurks a slow painting bug somewhere but I think caching scrollable areas is something we would end up doing anyways. If it is good for dynamic remote content it should be good for our handful of usecases. The pixelformat conversion mentioned in the comment you linked does not really apply on maemo where the locked texture dictates the pixel format the painting surface is created with.

> This patch saves not only uploading, but also re-flowing due to a moving > scrollbar and painting due to scrolled content as well as the scrollbar > needing to be painted onto the content. I can't rule out that there still > lurks a slow painting bug somewhere but I think caching scrollable areas is > something we would end up doing anyways. If it is good for dynamic remote > content it should be good for our handful of usecases. > The pixelformat conversion mentioned in the comment you linked does not > really apply on maemo where the locked texture dictates the pixel format the > painting surface is created with. The good news is that we are fixing the reflowing issues in a separate bug, yes? Do you know if that bug helps with the FPS here? It's not the pixel format I was linking to. It was the speed. I'm curious what texture uploading speed is like on maemo and if uploading is our bottleneck. I seem to recall it was background painting that was the bottleneck in the last profile you did. Anyways, maybe I'm too worried about the extra memory thrashing and certainly this will help a lot in the common use cases you've patched. I wonder what the plan is for the bookmarks and about:config though?

(In reply to Benjamin Stover (:stechz) from comment #55) > The good news is that we are fixing the reflowing issues in a separate bug, > yes? Do you know if that bug helps with the FPS here? I'm not sure, this bug depends on bug 524925, where we fix some translation issues. Without this patch we do SetRect which will always reflow at least the scrollbar as well as invalidate anything under it. > > It's not the pixel format I was linking to. It was the speed. I'm curious > what texture uploading speed is like on maemo and if uploading is our > bottleneck. I seem to recall it was background painting that was the > bottleneck in the last profile you did. I don't think it is the locking/unlocking we do on maemo in particular. We should be fast enough there. I guess it is the sum of things we do in scrolling plus that it is all happening synchronously, blocking user input. Here's some timing output from inside DrawThebesLayer in FrameLayerBuilder, as well as TiledTextureImage. destBufferRect:0,0 854x480 drawBounds: 846,75 6x397 ->DrawThebesLayer <-DrawThebesLayer 641 usec destroying texture: 854x405 destBufferRect:0,189 854x405 drawBounds: 0,189 854x405 created Texture: 854x405 ->DrawThebesLayer <-DrawThebesLayer 33508 usec destBufferRect:846,146 6x141 drawBounds: 846,146 6x141 created Texture: 6x141 destroying texture: 6x141 ->DrawThebesLayer <-DrawThebesLayer 1221 usec destBufferRect:0,199 854x396 drawBounds: 0,199 854x265 created Texture: 854x395 destroying texture: 854x396 ->DrawThebesLayer <-DrawThebesLayer 3632 usec destBufferRect:0,0 854x480 drawBounds: 846,89 6x383 ->DrawThebesLayer <-DrawThebesLayer 1191 usec destroying texture: 854x405 destBufferRect:0,218 854x405 drawBounds: 0,218 854x405 created Texture: 854x405 ->DrawThebesLayer <-DrawThebesLayer 41626 usec destBufferRect:846,157 6x141 drawBounds: 846,157 6x141 created Texture: 6x141 destroying texture: 6x141 ->DrawThebesLayer <-DrawThebesLayer 1282 usec destBufferRect:0,218 854x395 drawBounds: 8,218 844x386 created Texture: 854x386 destroying texture: 854x395 ->DrawThebesLayer <-DrawThebesLayer 3662 usec destBufferRect:0,0 854x480 drawBounds: 0,75 854x405 ->DrawThebesLayer <-DrawThebesLayer 732 usec destroying texture: 854x405 destBufferRect:0,127 854x405 drawBounds: 0,127 854x405 created Texture: 854x405 ->DrawThebesLayer <-DrawThebesLayer 34729 usec destBufferRect:846,123 6x141 drawBounds: 846,123 6x141 created Texture: 6x141 destroying texture: 6x141 ->DrawThebesLayer <-DrawThebesLayer 1251 usec destBufferRect:0,142 854x390 drawBounds: 0,142 854x272 created Texture: 854x390 destroying texture: 854x386 ->DrawThebesLayer <-DrawThebesLayer 4699 usec destBufferRect:0,0 854x480 drawBounds: 846,96 6x376 ->DrawThebesLayer <-DrawThebesLayer 702 usec destroying texture: 854x405 destBufferRect:0,211 854x405 drawBounds: 0,211 854x405 created Texture: 854x405 ->DrawThebesLayer <-DrawThebesLayer 42388 usec destBufferRect:846,154 6x141 drawBounds: 846,154 6x141 created Texture: 6x141 destroying texture: 6x141 ->DrawThebesLayer <-DrawThebesLayer 1251 usec destBufferRect:0,211 854x393 drawBounds: 8,211 846x393 created Texture: 854x393 destroying texture: 854x390 ->DrawThebesLayer <-DrawThebesLayer 3570 usec destBufferRect:0,0 854x480 drawBounds: 846,75 6x397 ->DrawThebesLayer <-DrawThebesLayer 671 usec destroying texture: 854x405 destBufferRect:0,86 854x405 drawBounds: 0,86 854x405 created Texture: 854x405 ->DrawThebesLayer <-DrawThebesLayer 35309 usec destBufferRect:846,107 6x141 drawBounds: 846,107 6x141 created Texture: 6x141 destroying texture: 6x141 ->DrawThebesLayer <-DrawThebesLayer 1251 usec destBufferRect:0,86 854x393 drawBounds: 0,86 854x328 created Texture: 854x378 destroying texture: 854x393 ->DrawThebesLayer <-DrawThebesLayer 3906 usec destBufferRect:0,0 854x480 drawBounds: 846,94 6x378 ->DrawThebesLayer <-DrawThebesLayer 702 usec looks like we are churning on texture images quite a bit > > Anyways, maybe I'm too worried about the extra memory thrashing and > certainly this will help a lot in the common use cases you've patched. I > wonder what the plan is for the bookmarks and about:config though? I guess for all cases with content potentially bigger than 2048px there should be a policy to move it by a sensible amount so we don't run into churning again.

I looked at the reasons why we were so slow in scrolling again in the first place. For each scroll movement there are 4 texture images involved. 1. The background receives an invalidation from the scrollbar although in the case of fennec nothing is happening there. lock, paint(?), unlock 2. The scrollbar thumb has its own texture image. Although it doesn't change, only move, it is being repainted because we do a crude SetRect and relayout for it. lock, paint, unlock. 3 & 4. The borderbackground and the text elements of the scrolled content each get their own image. This is because they each have a separate clip and between the clips the scroll thumb prevents these from merging. Due to the buffers being exactly the visible size we also seem to run into this condition here quite often, which means more buffer destructions and re-creations: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#522 Anyways, this nets us another 2x (buffer lock, paint, unlock)

So if we take your patches to fix the z-ordering of the thumb, and make it use CSS transforms, most of those problems would go away, right? We'd still have one update to the scrolled content image left. Let's break those patches out and land them.

what this patch was originally about

Comment on attachment 561486 [details] [diff] [review] enable displayport buffering for some common scrollboxes on fennec >+ winCwu.setDisplayPortForElement(0, 0, 2048, 2048, document.getElementById("prefs-list")._scrollbox); >+ winCwu.setDisplayPortForElement(0, 0, 2048, 2048, document.getElementById("addons-list")._scrollbox); >+ winCwu.setDisplayPortForElement(0, 0, 2048, 2048, document.getElementById("downloads-list")._scrollbox); >+ winCwu.setDisplayPortForElement(0, 0, 2048, 2048, document.getElementById("console-box")._scrollbox); I think we should also add document.getElementById("tabs")._scrollbox to this list (especially important in the new tablet layout).

Comment on attachment 561485 [details] [diff] [review] make scrollbar thumbs use css transforms on mobile Review of attachment 561485 [details] [diff] [review]: ----------------------------------------------------------------- r+ with those fixed. ::: layout/xul/base/src/nsSliderFrame.cpp @@ +388,5 @@ > + transform.AppendLiteral("px)"); > + result = css2->SetMozTransform(transform); > + > + nsIFrame* frame = mThumbContent->GetPrimaryFrame(); > + if(frame) { if (frame) { @@ +390,5 @@ > + > + nsIFrame* frame = mThumbContent->GetPrimaryFrame(); > + if(frame) { > + frame->InvalidateTransformLayer(); > + } Actually, this InvalidateTransformLayer call chould be unnecessary. The style change should take care of it. @@ +482,5 @@ > + LayoutChildAt(aState, thumbBox, nsRect(0,0, thumbSize.width, thumbSize.height)); > + SyncLayout(aState); > + // Redraw only if thumb changed size. > + if (oldThumbRect.Size() != thumbRect.Size()) > + Redraw(aState); {} @@ +485,5 @@ > + if (oldThumbRect.Size() != thumbRect.Size()) > + Redraw(aState); > + > + nsIFrame* thumbFrame = mFrames.FirstChild(); > + if(thumbFrame) { if (thumbFrame) {

(In reply to Matt Brubeck (:mbrubeck) from comment #62) > Comment on attachment 561486 [details] [diff] [review] > enable displayport buffering for some common scrollboxes on fennec > > >+ winCwu.setDisplayPortForElement(0, 0, 2048, 2048, document.getElementById("prefs-list")._scrollbox); > >+ winCwu.setDisplayPortForElement(0, 0, 2048, 2048, document.getElementById("addons-list")._scrollbox); > >+ winCwu.setDisplayPortForElement(0, 0, 2048, 2048, document.getElementById("downloads-list")._scrollbox); > >+ winCwu.setDisplayPortForElement(0, 0, 2048, 2048, document.getElementById("console-box")._scrollbox); > > I think we should also add document.getElementById("tabs")._scrollbox to > this list (especially important in the new tablet layout). I found that gecko doesn'T like display port an every element, so we have to try and see what works. For instance I have been trying to track down why setting a displayport on #controls-scrollbox breaks the autocomplete popup (bug 684881).

Comment on attachment 561486 [details] [diff] [review] enable displayport buffering for some common scrollboxes on fennec I don't like this one bit. It's too fragile and too obtrusive. If this is supposed to be added to <scrollbox>s then add it to the XBL and provide a way to set the buffering size. This patch just looks like a big hack. How much memory are we going to start chewing up because of this?

Comment on attachment 561484 [details] [diff] [review] fix: header buttons into separate stacking context I don't mind this change, but the CSS does not convey the end result. Very fragile. I wouldn't doubt this change would be "cleaned up" in a future refactoring without ever knowing the purpose. Is this really the best answer we have?

Let me pull back a bit and say that this looks like a promising way to speed up scrolling, but I don't think it's quite ready yet. The implementation aspects of the patches could be taken a bit further along and made a little cleaner for front-end consumers like Fennec.

> How much memory are we going to start chewing up because of this? already answered above see comment 53..

> fragile. I wouldn't doubt this change would be "cleaned up" in a future > refactoring without ever knowing the purpose. Any other suggestion how to fix this problem, I think UI should care from time to time about engine implementation bottle necks and find ways to build UI in such way that backend implementation will work in most optimal and fast way... Recall me some previous bug about getBoundingRects usage in UI.

(In reply to Oleg Romashin (:romaxa) from comment #68) > > How much memory are we going to start chewing up because of this? > already answered above see comment 53.. The preferences panel uses at least 3 scrollboxes (could be 4 if error console is on), Would memory be used for all 3 or only the visible one?

(In reply to Oleg Romashin (:romaxa) from comment #69) > > fragile. I wouldn't doubt this change would be "cleaned up" in a future > > refactoring without ever knowing the purpose. > Any other suggestion how to fix this problem, I think UI should care from > time to time about engine implementation bottle necks and find ways to build > UI in such way that backend implementation will work in most optimal and > fast way... > Recall me some previous bug about getBoundingRects usage in UI. Frankly, I get worried whenever the front-end needs to actively handle performance bottlenecks. Your getBoundingClientRect example is a perfect use case: Do you know how many times JS code has added gBCR since you filed that bug? We would need some static code analyzer to make sure we don't introduce a "badly" performing function call in some routine patch in the future. It's fragile.

Maybe we can put z-index:0 around the content <browser> to ensure that the scrollbars are captured in its stacking context.

(In reply to Mark Finkle (:mfinkle) from comment #70) > The preferences panel uses at least 3 scrollboxes (could be 4 if error > console is on), Would memory be used for all 3 or only the visible one? only actually visible elements get layers, also in stacks.