1417701 - MotionMark Focus20 spends a bunch of time in nsIFrame::DoUpdateStyleOfOwnedAnonBoxes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

Anonymous boxes seem like an inappropriate place to spend time on this test:


http://browserbench.org/MotionMark/developer.html?test-interval=30&display=minimal&tiles=big&controller=fixed&frame-rate=50&kalman-process-error=1&kalman-measurement-error=4&time-measurement=performance&suite-name=HTMLsuite&test-name=Focus20&complexity=64

https://perfht.ml/2z5hw0e

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Just for reference, I took a quick look and these are all :-moz-scrolled-content anon boxes...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

And there's one for each of the #scene > div elements, which have position: absolute and overflow: hidden...

It's not clear to me why we need scrollframes for overflow: hidden elements... I guess you can technically scroll them with JS setting scrollTop or what not? If so, that's very unfortunate.

In any case, there's no clear general optimization that we can do off-hand to optimize out style resolution for the scroll-frame, it'd need to be kind of a hack, in the sense that the scrollframe definitely inherits (and even inherits reset style) from the parent element, so we don't know off-hand whether the scrollframe style may or may not change...

We could hardcode the properties it touches and compare them with the old and new style of the element, or something like that... But that's kind of annoying, and looks fragile too.

Ideally we wouldn't construct scrollframes for this stuff to begin with...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I need to look at what does WebKit do for this. Meanwhile, I'll remove a couple of useless rules in there that should speed this up a bit.

Daniel, do you know if we could do something to not have the overhead of scrollcontent with overflow: hidden? I mean, I expect we need the scrollframe for all sorts of DOM APIs... But it'd be nice if we could do better.

Also, Markus and I were wondering why was reframing on overflow changes necessary? I guess to handle the IsScrollableOverflow() to !IsScrollableOverflow() change? If so, couldn't we do better at least for changes from auto to none and such?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1417701: Remove some useless rules and importance declarations in ua.css.

There's no need to set inheriting anon boxes reset properties to their defaults.

Also, there's no need to use !important, since they have no other rules. There's
::-moz-pagecontent which is tricky, because it can be affected by @page rules,
but only margin properties are allowed on those.

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

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> 
> It's not clear to me why we need scrollframes for overflow: hidden
> elements... I guess you can technically scroll them with JS setting
> scrollTop or what not? If so, that's very unfortunate.

Yes, I think that's the reason.  (overflow:-moz-overflow-clip avoids that.)  I wonder if we could avoid creating the scroll frame initially, then just reframe if someone touches scrollTop etc.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8929140 [details]
Bug 1417701: Remove some useless rules and importance declarations in ua.css.

https://reviewboard.mozilla.org/r/200430/#review205780

r=me with the -moz-box-ordinal-group declaration left unless there's a good reason to remove it.

::: commit-message-0f8c5:3
(Diff revision 1)
> +Bug 1417701: Remove some useless rules and importance declarations in ua.css. r?heycam
> +
> +There's no need to set inheriting anon boxes reset properties to their defaults.

You're also updating some non-inheriting anonymous box rules (like ::-moz-table-col-group), but that's also the right thing to do, so s/inheriting //.

::: layout/style/res/ua.css
(Diff revision 1)
>  
>  *|*::-moz-xul-anonymous-block {
> -  display: block ! important;
> -  position: static ! important;
> +  display: block;
> +  -moz-box-ordinal-group: inherit;
> -  float: none ! important;
> -  -moz-box-ordinal-group: inherit !important;

Should this one really be removed?  ::-moz-xul-anonymous-block is an inheriting anonymous box so this declaration could have an effect.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8929140 [details]
Bug 1417701: Remove some useless rules and importance declarations in ua.css.

https://reviewboard.mozilla.org/r/200430/#review205780

> Should this one really be removed?  ::-moz-xul-anonymous-block is an inheriting anonymous box so this declaration could have an effect.

It's not removed, only `!important` is removed.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8929140 [details]
Bug 1417701: Remove some useless rules and importance declarations in ua.css.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a45f9d4bb6a
Remove some useless rules and importance declarations in ua.css. r=heycam

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/9a45f9d4bb6a

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> Daniel, do you know if we could do something to not have the overhead of
> scrollcontent with overflow: hidden? I mean, I expect we need the
> scrollframe for all sorts of DOM APIs... But it'd be nice if we could do
> better.

(Not sure, sorry.  Looks like you found a solution here, though. Hooray!)