See the profiles in bug 1360889 comment 5 and the ensuing discussion.

(In reply to Emilio Cobos Álvarez [:emilio] from bug 1360889 comment 6) > I think the biggest offender there now is the font-size stuff. I think we > should fix that too. > > Manish, you wrote in a comment that you need to cascade font-size regardless > of whether it was specified to handle mathml's script min size and language > changes. > > Both of those are super-rare, is there a reason that shouldn't be guarded by > the seen bitfield? Seems quite wasteful to cascade all the time > unnecessarily. NI Manish.

We can use the seen bitfield to reduce the amount of fontsize crazyness, but not remove it completely. The unconstrained size stuff for example needs to always propagate correctly. If we wish to address that we have to store a "haa any parent been affected by scriptlevel" field on ComputedValues which is kinda wasteful. Could you be more specific as to what exactly is slow? The minsize stuff already bails early IIRC.

Oh, wait, I see what you mean. Yeah this can be short circuited.

I'd expect something like https://treeherder.mozilla.org/#/jobs?repo=try&revision=71498b89b9dda7f0ff32f9546883f66d97c54529 to work, let's see.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4) > I'd expect something like > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=71498b89b9dda7f0ff32f9546883f66d97c54529 to work, > let's see. Nice! Got some unexpected mathml passes in there. I wonder why?

This is because when we inherit, we're still scaling by the fraction stored in ComputedValues::font_size_keyword, which seems wrong to me. I'd defer to Manish on this, since I have no clue how the mathml mapped props are supposed to work. Given my patch is an strict improvement, I'm going to submit it for review, and flag manish on a followup bug to fix this (my patch just makes it correct in more cases, but not totally correct :)). Submitted it to https://github.com/servo/servo/pull/16682

I'm r+ing the PR at in comment 6 because this doesn't cause any test failure (and causes some test passes), and I want to get this into the tree to measure the impact on memory. I'm not entirely sure about this font stuff though, so I'd like Manishearth to take a look at the patch. If it's wrong somehow, we can fix it as a followup.

Er, measure the impact on performance (vis a vis allocator overhead).

Yeah that PR is a valid fix here. So we knowingly handle kw/em units in mathml differently here. When you apply scriptlevel to an em unit size that was applied to a keyword size and then change the language or generic font family, gecko will do the ratio thing to both the unconstrained and regular size. This is because Gecko handles the em-kw-generic confluence by just walking up the rule tree and recalculating the nsFont as if the generic was always the new one. I didn't want to cascade two keyword sizes (once we implement text-zoom that brings us to a grand total of five separate font-sizes being cascaded!), and this was very niche use case requiring the confluence of basically all the font-size complexity. Stylo will still do something reasonable here, just not the same as Gecko. So I discussed with dbaron and we were fine with changing this behavior. My plan was to implement this if we had too many tests affected by this, which we'd determine later once all the other things were fixed. Low priority. In this design I didn't realize that this also changes the behavior on inheriting a keyword size when applying scriptlevel. We currently unconditionally "inherit" the keyword font size that's stored alongside unconditionally. However, since we do not handle the kw changes when scriptlevel is applied, we should not be storing it as it's no longer accurate (or we should apply scriptlevel to the ratio there too, but this gets complicated again). The following patch fixes this issue and possibly another one which I noticed. I'll make some trypushes in a bit. diff --git a/servo/components/style/properties/gecko.mako.rs b/servo/components/style/properties/gecko.mako.rs index 3c2ab72..704cea3 100644 --- a/servo/components/style/properties/gecko.mako.rs +++ b/servo/components/style/properties/gecko.mako.rs @@ -1503,11 +1503,12 @@ fn static_assert() { /// so should not be called when you just want the font sizes to be copied. /// Hence the different name. pub fn inherit_font_size_from(&mut self, parent: &Self, - kw_inherited_size: Option<Au>) { + kw_inherited_size: Option<Au>) -> bool { let (adjusted_size, adjusted_unconstrained_size) = self.calculate_script_level_size(parent); if adjusted_size.0 != parent.gecko.mSize || - adjusted_unconstrained_size.0 != parent.gecko.mScriptUnconstrainedSize { + adjusted_unconstrained_size.0 != parent.gecko.mScriptUnconstrainedSize || + parent.gecko.mSize != parent.gecko.mScriptUnconstrainedSize { // This is incorrect. When there is both a keyword size being inherited // and a scriptlevel change, we must handle the keyword size the same // way we handle em units. This complicates things because we now have @@ -1525,18 +1526,21 @@ fn static_assert() { self.gecko.mFont.size = adjusted_size.0; self.gecko.mSize = adjusted_size.0; self.gecko.mScriptUnconstrainedSize = adjusted_unconstrained_size.0; + false } else if let Some(size) = kw_inherited_size { // Parent element was a keyword-derived size. self.gecko.mFont.size = size.0; self.gecko.mSize = size.0; // MathML constraints didn't apply here, so we can ignore this. self.gecko.mScriptUnconstrainedSize = size.0; + true } else { // MathML isn't affecting us, and our parent element does not // have a keyword-derived size. Set things normally. self.gecko.mFont.size = parent.gecko.mFont.size; self.gecko.mSize = parent.gecko.mSize; self.gecko.mScriptUnconstrainedSize = parent.gecko.mScriptUnconstrainedSize; + false } } diff --git a/servo/components/style/properties/longhand/font.mako.rs b/servo/components/style/properties/longhand/font.mako.rs index 3eeabec..d4975b7 100644 --- a/servo/components/style/properties/longhand/font.mako.rs +++ b/servo/components/style/properties/longhand/font.mako.rs @@ -921,10 +921,14 @@ ${helpers.single_keyword_system("font-variant-caps", let kw_inherited_size = context.style().font_size_keyword.map(|(kw, ratio)| { SpecifiedValue::Keyword(kw, ratio).to_computed_value(context) }); - context.mutate_style().mutate_font() + let used_kw = context.mutate_style().mutate_font() .inherit_font_size_from(parent, kw_inherited_size); - context.mutate_style().font_size_keyword = - context.inherited_style.font_size_keyword; + if used_kw { + context.mutate_style().font_size_keyword = + context.inherited_style.font_size_keyword; + } else { + context.mutate_style().font_size_keyword = None; + } } pub fn cascade_initial_font_size(context: &mut Context) {

Note that one MQ test started failing from this, which I'll mark as an expected fail. https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=95773028

Oh, my mistake, that reduced the number of failures.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b377cc24919a Adjust test expectations. r=me