Looking at profiles bug 1420423 comment 56, the most outstanding thing is that, we are resetting default computed values each time the window is resized. Under the "PresShell::DoFlushPendingNotifications Style", ComputedValues::default_values takes 120ms, and Arc<ComputedValues>::drop_slow takes 92ms, which sums up to 212ms. It takes over half of the regression in tresize.

According to the comment at https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/servo/ports/geckolib/glue.rs#1139-1140 bug 1369984 can be related.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0) > Under the "PresShell::DoFlushPendingNotifications Style", > ComputedValues::default_values takes 120ms, and > Arc<ComputedValues>::drop_slow takes 92ms, which sums up to 212ms. It takes > over half of the regression in tresize. Wow. Yeah, so the key here is that if you use em / rem / other relative units in media queries, they get resolved against the default font-size. The default font-size depends on the zoom, and we don't know if the media-query change is due to a zoom change or not. I also noticed that this has non-trivial performance implications in the sense that it will make the next restyle also a lot more expensive (because suddenly basically _all_ style structs are pointer-unequal, which means that we'll spend a lot of time in CalcStyleDifference). This should be an easy-ish fix.

I'm working on this btw, I just got into a very deep rabbit hole :(

Comment on attachment 8948742 [details] Bug 1435939: Make media feature changes always async. https://reviewboard.mozilla.org/r/218138/#review224008 ::: layout/base/nsPresContext.h:477 (Diff revision 1) > - PostMediaFeatureValuesChangedEvent(); > + nsRestyleHint(0), > + nsChangeHint(0), Why not use the one-arg version here? ::: layout/style/MediaFeatureChange.h:19 (Diff revision 1) > + ViewportChange = 1 << 0, > + ZoomChange = 1 << 1, > + MinFontSizeChange = 1 << 2, > + ResolutionChange = 1 << 3, > + MediumChange = 1 << 4, > + SizeModeChange = 1 << 5, > + SystemMetricsChange = 1 << 6, > + DeviceSizeIsPageSizeChange = 1 << 7, > + DisplayModeChange = 1 << 8, These should probably be documented some. ::: layout/style/nsComputedDOMStyle.cpp:161 (Diff revision 1) > StyleSetHandle styleSet = shell->StyleSet(); > if (styleSet->StyleSheetsHaveChanged()) { > return true; > } > > + // Pending media query updates can definitely change style on the element, so You mean due to script running that would update the style? Or because of something else? If it's because of script running, we shouldn't worry about that here.... Would be good to clarify this comment no matter what. ::: layout/style/nsComputedDOMStyle.cpp:162 (Diff revision 1) > if (styleSet->StyleSheetsHaveChanged()) { > return true; > } > > + // Pending media query updates can definitely change style on the element, so > + // gotta flush. This part shouldgo _inside_ the if, no? ::: layout/style/nsComputedDOMStyle.cpp:963 (Diff revision 1) > + // FIXME(emilio): This doesn't look sound to me, the parent flush may actually > + // run script or what not in a way that causes the element to need a > + // restyle... Well, in an ideal world we would not run any script here at all... But in any case, if script runs in this flush and that affects the computed style, but then we return the pre-script-running styles, that's probably more or less OK. Because ideally that script would not have run there anyway.

Comment on attachment 8948742 [details] Bug 1435939: Make media feature changes always async. https://reviewboard.mozilla.org/r/218138/#review224010 ::: layout/base/nsPresContext.h:297 (Diff revision 1) > * > * For aChangeHint, see RestyleManager::RebuildAllStyleData. (Passing > * a nonzero aChangeHint forces rebuilding style data even if > * nsRestyleHint(0) is passed.) > */ > - void MediaFeatureValuesChanged(nsRestyleHint aRestyleHint, > + void MediaFeatureValuesChanged(const mozilla::MediaFeatureChange& aChange) I think passing the argument by value in this case is acceptable, given that you are constructing a temporary object in callsites and the struct itself isn't that large. ::: layout/style/MediaFeatureChange.h:38 (Diff revision 1) > +{ > + nsRestyleHint mRestyleHint; > + nsChangeHint mChangeHint; > + MediaFeatureChangeReason mReason; > + > + MediaFeatureChange(MediaFeatureChangeReason aReason) You would need to mark this either `explicit`, or `MOZ_IMPLICIT` (the latter is acceptable in my opinion, though, and in that way you may be able to avoid wrapping some of reasons expression in `{}` when passing it down).

Comment on attachment 8948742 [details] Bug 1435939: Make media feature changes always async. https://reviewboard.mozilla.org/r/218138/#review224014 ::: layout/base/nsPresContext.h:477 (Diff revision 1) > - PostMediaFeatureValuesChangedEvent(); > + nsRestyleHint(0), > + nsChangeHint(0), I added the one-arg version after writing this patch :). Will fix. ::: layout/style/MediaFeatureChange.h:19 (Diff revision 1) > + ViewportChange = 1 << 0, > + ZoomChange = 1 << 1, > + MinFontSizeChange = 1 << 2, > + ResolutionChange = 1 << 3, > + MediumChange = 1 << 4, > + SizeModeChange = 1 << 5, > + SystemMetricsChange = 1 << 6, > + DeviceSizeIsPageSizeChange = 1 << 7, > + DisplayModeChange = 1 << 8, Will do. ::: layout/style/MediaFeatureChange.h:38 (Diff revision 1) > +{ > + nsRestyleHint mRestyleHint; > + nsChangeHint mChangeHint; > + MediaFeatureChangeReason mReason; > + > + MediaFeatureChange(MediaFeatureChangeReason aReason) Thanks, indeed. I think I prefer to keep the wrapping because it makes obvious the fact that you're constructing an object... But will give it a thought while in bed tonight :) ::: layout/style/nsComputedDOMStyle.cpp:161 (Diff revision 1) > StyleSetHandle styleSet = shell->StyleSet(); > if (styleSet->StyleSheetsHaveChanged()) { > return true; > } > > + // Pending media query updates can definitely change style on the element, so No, just because it changes the rules that may apply to the element. For example if you resize an iframe, then call getComputedStyle into that iframe, and that iframe has a media query, you should be able to observe it with the new size. Will reword to clarify. ::: layout/style/nsComputedDOMStyle.cpp:162 (Diff revision 1) > if (styleSet->StyleSheetsHaveChanged()) { > return true; > } > > + // Pending media query updates can definitely change style on the element, so > + // gotta flush. Yup ::: layout/style/nsComputedDOMStyle.cpp:963 (Diff revision 1) > + // FIXME(emilio): This doesn't look sound to me, the parent flush may actually > + // run script or what not in a way that causes the element to need a > + // restyle... Ok, fair enough... Will remove the comment for now I guess :)

> For example if you resize an iframe, then call getComputedStyle into that iframe, and that > iframe has a media query, you should be able to observe it with the new size. Agreed. Testcase needed?

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11) > > For example if you resize an iframe, then call getComputedStyle into that iframe, and that > > iframe has a media query, you should be able to observe it with the new size. > > Agreed. Testcase needed? This was caught for test_media_queries_dynamic.html (and also test_display_mode.html iirc)

s/for/by

Comment on attachment 8948743 [details] Bug 1435939: Propagate the media feature change reason around. https://reviewboard.mozilla.org/r/218140/#review224034 ::: dom/xbl/nsBindingManager.h:143 (Diff revision 1) > // Do any processing that needs to happen as a result of a change in the > // characteristics of the medium, and return whether this rule processor's > // rules or the servo style set have changed (e.g., because of media > // queries). > - bool MediumFeaturesChanged(nsPresContext* aPresContext); > + bool MediumFeaturesChanged(nsPresContext* aPresContext, > + mozilla::MediaFeatureChangeReason); I would propably prefer you to give the param a name (here and several places below), maybe just `aReason`. Although the type is clear enough for what it is, a param without name gives me an impression that it is unused. But it's not a big issue, so it's up to you.

Comment on attachment 8948744 [details] Bug 1435939: Don't reset the default computed values if they cannot change. https://reviewboard.mozilla.org/r/218142/#review224036 ::: layout/style/ServoStyleSet.cpp:294 (Diff revision 1) > } > > return nsRestyleHint(0); > } > > +static const MediaFeatureChangeReason kMediaFeaturesAffectingDefaultStyle = It seems to me that `ResolutionChange` may also affect default style, given that `nsStyleOutline` and `nsStyleColumn` store the appunit-per-devpx. But maybe that doesn't matter because they are not useful when the struct doesn't contain meaningful length, and we would construct a brave new instance for any thing which contains such length, is my understanding correct? If that's the case, it's probably worth at least having a comment here mentioning this situation that we may have staled style structs depending on the old resolution, but we don't care. ::: servo/ports/geckolib/glue.rs:1150 (Diff revision 1) > - } > + if may_affect_default_style { > + // FIXME(emilio): It's a shame we do this too for XBL stuff, but we need > + // to right now to evaluate relative units in media queries correctly. > + // > + // We should instead just pass the `Device` reference from the master > + // stylist, but that looked kinda gross... Maybe it's not actually. Maybe it's not actually *what*?

Comment on attachment 8948745 [details] Bug 1435939: Process all the MediumFeatureChanges at the same time. https://reviewboard.mozilla.org/r/218144/#review224048 The XBL StyleSet part looks pretty hacky. So with this patch, we are not using the device of XBL stylist at all after the initial `UpdateStylist`. We use master's device for media values check, and drop the old stylist when anything changes, so that we create a new XBL stylist with the correct device info. Is my understanding correct? Actually this stuff looks hacky before this change as well. If I understand correctly, we were also kicking the XBL prototype back and forth between different document when someone has media feature values change? (It took me quite a while to understand the bits here, since I'm not very familiar with this code. Actually I still don't have enough confidence here, but the refactor looks reasonable... so) r=me with the nits fixed. ::: layout/style/ServoBindingList.h:94 (Diff revision 1) > // We'd like to return `OriginFlags` here, but bindgen bitfield enums don't > // work as return values with the Linux 32-bit ABI at the moment because > // they wrap the value in a struct. > SERVO_BINDING_FUNC(Servo_StyleSet_SetDevice, uint8_t, > RawServoStyleSetBorrowed set, RawGeckoPresContextOwned pres_context) I believe you can remove this now. ::: layout/style/ServoBindings.h:150 (Diff revision 1) > + bool affects_document_rules; > + bool affects_non_document_rules; > + bool uses_viewport_units; Since it's still a C++ struct, it probably makes more sense to use Gecko naming convention that `mAffectsDocumentRules` etc. It looks weird to mix the two naming conventions in C++ code. ::: layout/style/ServoStyleSet.h:448 (Diff revision 1) > // Set PresContext (i.e. Device) for mRawSet. This should be called only > // by XBL stylesets. Returns true if there is any rule changing. > bool SetPresContext(nsPresContext* aPresContext); This is not needed anymore as well, right? ::: servo/components/style/stylist.rs:1092 (Diff revision 1) > /// > /// For Gecko, this is called when XBL bindings are used by different > /// documents. This doesn't seem to apply anymore. ::: servo/components/style/stylist.rs:1129 (Diff revision 1) > + /// Passing `device` is needed because this is used for XBL in Gecko, where > + /// the `device` in those Stylist is used. It sounds unclear what "those Stylist" is, and the intuitive is probably actually wrong given the code below. It should probably say that the `device` in the stylist of the master document is used.

Comment on attachment 8948744 [details] Bug 1435939: Don't reset the default computed values if they cannot change. https://reviewboard.mozilla.org/r/218142/#review224036 > It seems to me that `ResolutionChange` may also affect default style, given that `nsStyleOutline` and `nsStyleColumn` store the appunit-per-devpx. > > But maybe that doesn't matter because they are not useful when the struct doesn't contain meaningful length, and we would construct a brave new instance for any thing which contains such length, is my understanding correct? > > If that's the case, it's probably worth at least having a comment here mentioning this situation that we may have staled style structs depending on the old resolution, but we don't care. Hmm, great catch. No, we won't construct anything new using the pres context (we'd use the copy-constructor on the initial struct). So the value could definitely go stale. Will fix. > Maybe it's not actually *what*? Maybe it's not [that gross] actually. Won't bother fixing this one because it goes away on the next patch.

Comment on attachment 8948745 [details] Bug 1435939: Process all the MediumFeatureChanges at the same time. https://reviewboard.mozilla.org/r/218144/#review224048 Yes, your understanding is correct. And yes, this setup is completely borked in various ways, but that's not anything that my patch changes at all... > Since it's still a C++ struct, it probably makes more sense to use Gecko naming convention that `mAffectsDocumentRules` etc. It looks weird to mix the two naming conventions in C++ code. Well, it's a rust struct in the other side... ;) But ok, will change it.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/12ca97022384 Make media feature changes always async. r=bz https://hg.mozilla.org/integration/autoland/rev/66535b923cac Propagate the media feature change reason around. r=xidorn https://hg.mozilla.org/integration/autoland/rev/4382ee96347b Don't reset the default computed values if they cannot change. r=xidorn https://hg.mozilla.org/integration/autoland/rev/12de5643ae0a Process all the MediumFeatureChanges at the same time. r=xidorn

https://hg.mozilla.org/mozilla-central/rev/12ca97022384 https://hg.mozilla.org/mozilla-central/rev/66535b923cac https://hg.mozilla.org/mozilla-central/rev/4382ee96347b https://hg.mozilla.org/mozilla-central/rev/12de5643ae0a