Closed Bug 1251075 Opened 4 years ago Closed 3 years ago

make nsChangeHint_UpdateContainingBlock cheaper when is-a-containing-block didn't actually change

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox47 --- affected
firefox51 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: perf)

Attachments

(3 files)

This is a followup to bug 1187851.

The gaia test failure that happened there pointed out the issue that nsChangeHint_UpdateContainingBlock is a little bit over-eager -- it does work when one of the properties that triggers being a containing block changes, but we really only need to do that work when the answer to "does this force a containing block" (which is determined from a combination of properties) actually changes.

We could fix this is one of two ways.

First, we could put the complexity in the style code by moving the difference computation out of the nsStyle*::CalcDifference and into nsStyleContext::CalcStyleDifference.  I think this is pretty tricky, though, because there are some other dependencies on nsStyle*::CalcDifference.

Alternatively, we could store the "is it a containing block" somewhere else (a frame state bit, perhaps, or maybe a frame property, or maybe a style context bit), and use that information as part of processing the nsChangeHint_UpdateContainingBlock hint.  This is probably a better approach.
Duplicate of this bug: 1298482
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #0)
> First, we could put the complexity in the style code by moving the
> difference computation out of the nsStyle*::CalcDifference and into
> nsStyleContext::CalcStyleDifference.  I think this is pretty tricky, though,
> because there are some other dependencies on nsStyle*::CalcDifference.

I actually don't remember what I meant by "some other dependencies" -- at least not for cases that wouldn't be satisfied by NeutralChange hints.

> Alternatively, we could store the "is it a containing block" somewhere else
> (a frame state bit, perhaps, or maybe a frame property, or maybe a style
> context bit), and use that information as part of processing the
> nsChangeHint_UpdateContainingBlock hint.  This is probably a better approach.

That said, it seems easy enough to store it as a style context bit, eagerly computed in FinishConstruction or ApplyStyleFixups, and then diff the bit in CalcStyleDifference to determine whether to return UpdateContainingBlock.

(In reply to Boris Zbarsky [:bz] from bug 1298482 comment #0)
> I'm not sure whether the right fix is to make nsStyleDisplay::CalcDifference
> smarter or whether the right fix is to make
> NeedToReframeForAddingOrRemovingTransform smarter...  Thoughts?

I'm not sure what you meant by making NeedToReframeForAddingOrRemovingTransform smarter.
> I'm not sure what you meant by making NeedToReframeForAddingOrRemovingTransform smarter.

Right now NeedToReframeForAddingOrRemovingTransform has this optimization where it checks the frame's position property and if it's absolute/fixed/relative it assumes the frame will still be an abs pos containing block and only looks for fixed-pos descendants to decide whether to reframe.  Of course this assumes that changes to "position" will reframe, which they do.

I guess I was thinking of checking more state to decide whether we can optimize out looking for fixed-pos too, but that doesn't really work because we have to check only state which will trigger a reframe if it's changed, and the whole goal here is to be checking otehr UpdateContainingBlock state.  So that idea doesn't really work.

I do still think that we can easily make nsStyleDisplay::CalcDifference somewhat smarter without any style context bits or whatnot.  Basically, factor out the non-frame-dependent pieces of IsAbsPosContainingBlock into a new nsStyleDisplay method and if that boolean tests true both before and after there is no reason to UpdateContainingBlock, right?  This will at least ensure that having "will-change" set appropriately means no reframes from transform changes...
Yeah, I think that should work as long as the CalcDifference methods return NeutralChange for the changes that aren't handled in CalcStyleDifference.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
The fact that HasTransform() depends on the frame requires the *Internal
functions rather than simply having the public function that takes a
frame call the public function that takes a style context.

Review commit: https://reviewboard.mozilla.org/r/74760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74760/
Attachment #8785619 - Flags: review?(bzbarsky)
Attachment #8785620 - Flags: review?(bzbarsky)
Attachment #8785621 - Flags: review?(bzbarsky)
Without the patch (or, rather, with the new code in nsStyleContext::CalcStyleDifferenceInternal commented out), the second of the tests added fails:
TEST-UNEXPECTED-FAIL | layout/style/test/test_change_hint_optimizations.html | adding a transform style to an element with filter: blur(3px); should not cause frame reconstruction even when the element has absolutely positioned descendants - got 34, expected 28

The first of the new tests doesn't fail without the patch because
will-change:transform influences HasTransformStyle.  However, since that
case is probably the most important one, it still seems worth testing
explicitly.

With the patches, both tests pass.

Review commit: https://reviewboard.mozilla.org/r/74764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74764/
Comment on attachment 8785619 [details]
Bug 1251075 - Split out the parts of nsStyleDisplay::Is{Abs,Fixed}PosContainingBlock that don't depend on the frame.

https://reviewboard.mozilla.org/r/74760/#review72878

::: layout/style/nsStyleStruct.h:3083
(Diff revision 1)
> +private:
> +  template<class StyleContextLike>
> +  inline bool HasAbsPosContainingBlockStyleInternal(
> +                StyleContextLike* aStyleContext) const;
> +  template<class StyleContextLike>
> +  inline bool HasFixedPosContainingBlockStyleInternal(

This could use a comment explaining that it doesn't check _all_ the styles that can lead something to be a fixed-pos containing block.  Similar for the abs pos version.

::: layout/style/nsStyleStructInlines.h:206
(Diff revision 1)
> +nsStyleDisplay::IsAbsPosContainingBlockForAppropriateFrame(StyleContextLike* aStyleContext) const
> +{
> +  // NOTE: Any CSS properties that influence the output of this function
> +  // should have the CSS_PROPERTY_ABSPOS_CB set on them.
> +  return HasAbsPosContainingBlockStyleInternal(aStyleContext) ||
> +         HasFixedPosContainingBlockStyleInternal(aStyleContext) ||

Why not replace the last two pieces with IsFixedPosContainingBlockForAppropriateFrame()?

r=me
Attachment #8785619 - Flags: review?(bzbarsky) → review+
Comment on attachment 8785620 [details]
Bug 1251075 - Optimize away nsChangeHint_UpdateContainingBlock in nsStyleContext::CalcStyleDifference when possible.

https://reviewboard.mozilla.org/r/74762/#review72882

::: layout/style/nsStyleContext.cpp:1267
(Diff revision 1)
>      if (change) {
>        hint |= nsChangeHint_RepaintFrame;
>      }
>    }
>  
> +  if (hint & nsChangeHint_UpdateContainingBlock) {

So the reason to do this in nsStyleContext::CalcDifference instead of inside nsStyleDisplay is because it depends on the StyleEffects struct, right?  Please document that.

r=me
Attachment #8785620 - Flags: review?(bzbarsky) → review+
Comment on attachment 8785621 [details]
Bug 1251075 - Add test that dynamic change of transform when will-change:transform is set doesn't reconstruct frames.

https://reviewboard.mozilla.org/r/74764/#review72884

::: layout/style/test/test_change_hint_optimizations.html:15
(Diff revision 1)
> +  SimpleTest.waitForExplicitFinish();
> +
> +  function runTests() {
> +
> +    /** Test for Bug 1251075 **/
> +    (function() {

Why is this immediately-executed funcion needed?  Seems like you can just get rid of it and outdent all the stuff inside it...

r=me.  Thank you for fixing this!
(In reply to Boris Zbarsky [:bz] from comment #11)
> Why is this immediately-executed funcion needed?  Seems like you can just
> get rid of it and outdent all the stuff inside it...

I wanted it that way so that we could add more tests to the same file without the names inside them interfering with each other.  I guess I could give the inner function a more specific name, though.
Comment on attachment 8785621 [details]
Bug 1251075 - Add test that dynamic change of transform when will-change:transform is set doesn't reconstruct frames.

bz said "r=me", but presumably forgot to mark it
Attachment #8785621 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d719224c81499b4a3e6755e9289d94229470884
Bug 1251075 - Split out the parts of nsStyleDisplay::Is{Abs,Fixed}PosContainingBlock that don't depend on the frame.  r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/c206d60dc0bf44960f566a534a36a0e6ae7d0b2b
Bug 1251075 - Optimize away nsChangeHint_UpdateContainingBlock in nsStyleContext::CalcStyleDifference when possible.  r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/38a29590c3e8c36155147aedd7b9e95466017877
Bug 1251075 - Add test that dynamic change of transform when will-change:transform is set doesn't reconstruct frames.  r=bz
Comment on attachment 8785621 [details]
Bug 1251075 - Add test that dynamic change of transform when will-change:transform is set doesn't reconstruct frames.

https://reviewboard.mozilla.org/r/74764/#review72916
Attachment #8785621 - Flags: review+
Depends on: 1302955
You need to log in before you can comment on or make changes to this bug.