1399228 - stylo: Move font computation data to style structs

Status: RESOLVED FIXED

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

Description

[Manish Goregaokar [:manishearth]]

We can simplify things by making the computed value of font-size into (Au, Option<keyword>, factor, offset) and storing the entire thing on nsStyleFont. This will also reduce the size of ComputedValues by 2 words (and potentially not increase the size of nsStyleFont by 2 words if we pack it correctly)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

When do we compute them then? And how would you resolve em units?

Comment 2

[Bobby Holley (:bholley)]

P3, but still very much want this.

Comment 3

[Manish Goregaokar [:manishearth]]

> When do we compute them then? And how would you resolve em units?

Compute it normally. In fact if not for the mathml stuff this simplifies font size computation to be like normal computed properties, where font-size's computed value has some extra data on it, and that data sometimes composes with the parent in the case of em/%/calc. And where inheriting through a language change triggers manual specification.


The interaction with mathml is tricky; I'm hoping to remove all those font-size special cascade functions but mathml may require some to be kept around.


(This was always the plan, just that I intended to do it after the gecko system was removed so we don't have an unnecessary impact on it)

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1399228 - Add keyword info to computed value of font-size;

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

Comment 5

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1399228 - stylo: Add font keyword info fields on nsStyleFont;

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

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1399228 - stylo: Remove FontComputationData, switch over to using the new font tracking;

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

Comment 7

[Manish Goregaokar [:manishearth]]

Might be possible to move stuff into StyleBuilder now, fwiw. I haven't attempted it.

This stuff seems to work now.

I was unable to remove the need for special casing entirely because mathml still needs it. However the inherit one can be removed if we want (I didn't because it also needs copy_font_size_from to exist, which it doesn't)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8908870 [details]
Bug 1399228 - Add keyword info to computed value of font-size;

https://reviewboard.mozilla.org/r/180496/#review185766

r=me with `KeywordInfo` in the `SpecifiedValue`, at least. Can be another commit on top of the series if you prefer.

::: servo/components/style/properties/longhand/font.mako.rs:642
(Diff revision 1)
>          fn from(other: specified::LengthOrPercentage) -> Self {
>              SpecifiedValue::Length(other)
>          }
>      }
>  
>      pub mod computed_value {

If while you're here you move it to `values/specified/font.rs` and `values/computed/font.rs`, that'd be neat.

::: servo/components/style/properties/longhand/font.mako.rs:677
(Diff revision 1)
> +                }
> +            }
> +        }
> +
> +        impl T {
> +            pub fn size(self) -> Au {

nit: This can take a reference.

::: servo/components/style/properties/longhand/font.mako.rs:950
(Diff revision 1)
> +                        info = parent.info.map(|i| i.compose(ratio, abs));
> +                    }
>                      let calc = calc.to_computed_value_zoomed(context, base_size);
>                      calc.to_used_value(Some(base_size.resolve(context))).unwrap().into()
>                  }
> -                SpecifiedValue::Keyword(ref key, fraction, offset) => {
> +                SpecifiedValue::Keyword(key, fraction, offset) => {

nit: This may look nicer if you rename the fields here to `kw, factor, offset`, or even better, if you   use `KeywordInfo` there directly.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8908871 [details]
Bug 1399228 - stylo: Add font keyword info fields on nsStyleFont;

https://reviewboard.mozilla.org/r/180498/#review185768

::: layout/style/nsStyleConsts.h:652
(Diff revision 1)
>  #define NS_STYLE_FONT_SIZE_XLARGE               5
>  #define NS_STYLE_FONT_SIZE_XXLARGE              6
>  #define NS_STYLE_FONT_SIZE_XXXLARGE             7  // Only used by <font size="7">. Not specifiable in CSS.
>  #define NS_STYLE_FONT_SIZE_LARGER               8
>  #define NS_STYLE_FONT_SIZE_SMALLER              9
> +#define NS_STYLE_FONT_SIZE_NO_KEYWORD          10 // Used by Servo to track the "no keyword" case

I'd make this `0`, and move the rest, but IIRC there are some places in Gecko which depend on the value, so if it's not trivial feel free to skip it.

::: servo/components/style/properties/gecko.mako.rs:2254
(Diff revision 1)
>  
>      pub fn set_font_size(&mut self, v: longhands::font_size::computed_value::T) {
> +        use self::longhands::font_size::KeywordSize;
>          self.gecko.mSize = v.size().0;
>          self.gecko.mScriptUnconstrainedSize = v.size().0;
> +        if let Some(info) = v.info {

I'd rename the `info` field in the computed value to `keyword_info`. Again, this can be another commit.

::: servo/components/style/properties/gecko.mako.rs:2433
(Diff revision 1)
>              // Keep track of the unconstrained adjusted size.
>              self.gecko.mSize = adjusted_size.0;
> +
> +            // technically the MathML constrained size may also be keyword-derived
> +            // but we ignore this since it would be too complicated
> +            // to correctly track and it's mostly unnecessary

Hmm... I guess?

Let's use a dot here for consistency, and capitalize `Technically`.

::: servo/components/style/properties/gecko.mako.rs:2444
(Diff revision 1)
>              self.fixup_font_min_size(device);
>              false
>          } else if let Some(size) = kw_inherited_size {
>              // Parent element was a keyword-derived size.
>              self.gecko.mSize = size.value();
> +            // copy keyword info over

ditto. and below too.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8908872 [details]
Bug 1399228 - stylo: Remove FontComputationData, switch over to using the new font tracking;

https://reviewboard.mozilla.org/r/180500/#review185764

r=me, with `as_font_ratio` killed, and nits fixed.

::: servo/components/style/properties/longhand/font.mako.rs
(Diff revision 1)
>      pub fn cascade_specified_font_size(context: &mut Context,
>                                         specified_value: &SpecifiedValue,
>                                         mut computed: computed_value::T) {
> -        if let SpecifiedValue::Keyword(kw, fraction, offset) = *specified_value {
> -            context.builder.font_size_keyword = Some((kw, fraction, offset));
> -        } else if let Some((ratio, abs)) = specified_value.as_font_ratio(context) {

`as_font_ratio` can go away now I believe.

::: servo/components/style/properties/longhand/font.mako.rs:1107
(Diff revision 1)
> -        let device = context.builder.device;
>          let mut font = context.builder.take_font();
> -        let used_kw = {
> -            let parent_font = context.builder.get_parent_font();
> -            parent_kw = *context.builder.inherited_font_computation_data();
> -
> +        font.inherit_font_size_from(context.builder.get_parent_font(),
> +                                    kw_inherited_size,
> +                                    context.builder.device);
> +        context.builder.put_font(font);   

nit: Trailing space.

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908870 [details]
Bug 1399228 - Add keyword info to computed value of font-size;

https://reviewboard.mozilla.org/r/180496/#review185766

> nit: This can take a reference.

it's Copy, not taking a reference is better

(usually the optimizer will go both ways)

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908871 [details]
Bug 1399228 - stylo: Add font keyword info fields on nsStyleFont;

https://reviewboard.mozilla.org/r/180498/#review185768

> I'd make this `0`, and move the rest, but IIRC there are some places in Gecko which depend on the value, so if it's not trivial feel free to skip it.

Yeah no gecko relies on the value in a bunch of places.

Comment 13

[Bobby Holley (:bholley)]

How risky is this? In today's quantum meeting, Joe requested that we defer landing further memory optimizations until 58 (i.e. next week) to reduce risk. These patches are pretty big, so given that our memory numbers are looking much better now it may be worth just parking them for a few days and then landing them after the merge.

Comment 14

[Manish Goregaokar [:manishearth]]

I don't think this is risky, provided the linux32 tests pass without crashing (that seems to be the case).

I probably can land this today, there's just some cleanup and a bugfix to do.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Manish Goregaokar [:manishearth] from comment #14)
> I don't think this is risky, provided the linux32 tests pass without
> crashing (that seems to be the case).
> 
> I probably can land this today, there's just some cleanup and a bugfix to do.

Per IRC discussion, we're going to land this on 58 immediately after the merge, request uplift, and let the drivers make the call.

Comment 16

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1399228 - stylo: Move font-size stuff to values::*::font ;

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

Comment 17

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1399228 - stylo: Clean up keyword values;

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

Comment 18

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908871 [details]
Bug 1399228 - stylo: Add font keyword info fields on nsStyleFont;

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

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908872 [details]
Bug 1399228 - stylo: Remove FontComputationData, switch over to using the new font tracking;

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

Comment 20

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908871 [details]
Bug 1399228 - stylo: Add font keyword info fields on nsStyleFont;

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

Comment 21

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908872 [details]
Bug 1399228 - stylo: Remove FontComputationData, switch over to using the new font tracking;

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

Comment 22

[Manish Goregaokar [:manishearth]]

Comment on attachment 8909501 [details]
Bug 1399228 - stylo: Move font-size stuff to values::*::font ;

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

Comment 23

[Manish Goregaokar [:manishearth]]

Comment on attachment 8909502 [details]
Bug 1399228 - stylo: Clean up keyword values;

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

Comment 24

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908872 [details]
Bug 1399228 - stylo: Remove FontComputationData, switch over to using the new font tracking;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180500/diff/3-4/

Comment 25

[Manish Goregaokar [:manishearth]]

Comment on attachment 8909501 [details]
Bug 1399228 - stylo: Move font-size stuff to values::*::font ;

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

Comment 26

[Manish Goregaokar [:manishearth]]

Comment on attachment 8909502 [details]
Bug 1399228 - stylo: Clean up keyword values;

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

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8909502 [details]
Bug 1399228 - stylo: Clean up keyword values;

https://reviewboard.mozilla.org/r/180980/#review186564

::: servo/components/style/values/specified/font.rs:317
(Diff revision 3)
>                  #[cfg(feature = "gecko")] {
>                      context.cached_system_font.as_ref().unwrap().font_size.size
>                  }
>              }
>          };
> -        computed::FontSize { size, info }
> +        computed::FontSize {

nit: I'd rename `info` to `keyword_info`, and keep using the shorthand init syntax, but your call.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8909501 [details]
Bug 1399228 - stylo: Move font-size stuff to values::*::font ;

https://reviewboard.mozilla.org/r/180978/#review186566

Haven't looked super-close, but assuming there's only code moving around, r=me with those.

::: servo/components/style/values/specified/font.rs:61
(Diff revision 3)
> +    fn from(other: LengthOrPercentage) -> Self {
> +        FontSize::Length(other)
> +    }
> +}
> +
> +use self::KeywordSize::*;

Let's move this to the functions that use it.

::: servo/components/style/values/specified/font.rs:194
(Diff revision 3)
> +            [9,   10,    13,    16,    18,    24,    32,    48]
> +        ];
> +
> +        static FONT_SIZE_FACTORS: [i32; 8] = [60, 75, 89, 100, 120, 150, 200, 300];
> +
> +        // XXXManishearth handle quirks mode

What is this about? I know this is pre-existing, but is there a bug on file for it? Is it fixed?

If not, mind opening, and referencing from here?

Comment 29

[Manish Goregaokar [:manishearth]]

Comment on attachment 8909501 [details]
Bug 1399228 - stylo: Move font-size stuff to values::*::font ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180978/diff/3-4/

Comment 30

[Manish Goregaokar [:manishearth]]

Comment on attachment 8909502 [details]
Bug 1399228 - stylo: Clean up keyword values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180980/diff/3-4/

Comment 31

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908870 [details]
Bug 1399228 - Add keyword info to computed value of font-size;

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

Comment 32

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908871 [details]
Bug 1399228 - stylo: Add font keyword info fields on nsStyleFont;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180498/diff/3-4/

Comment 33

[Manish Goregaokar [:manishearth]]

Comment on attachment 8908872 [details]
Bug 1399228 - stylo: Remove FontComputationData, switch over to using the new font tracking;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180500/diff/4-5/

Comment 34

[Manish Goregaokar [:manishearth]]

Comment on attachment 8909501 [details]
Bug 1399228 - stylo: Move font-size stuff to values::*::font ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180978/diff/4-5/

Comment 35

[Manish Goregaokar [:manishearth]]

Comment on attachment 8909502 [details]
Bug 1399228 - stylo: Clean up keyword values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180980/diff/4-5/

Comment 36

[Manish Goregaokar [:manishearth]]

rebased, will land tomorrow

Comment 37

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/18607

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1287ef3bbba4
stylo: Add font keyword info fields on nsStyleFont; r=emilio
https://hg.mozilla.org/integration/autoland/rev/284f9a059a93
stylo: Remove FontComputationData, switch over to using the new font tracking; r=emilio

Comment 39

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/18609 is a fixup for fixing the orange in layout/style/test/test_transitions_per_property.html

Comment 40

[Manish Goregaokar [:manishearth]]

https://hg.mozilla.org/integration/autoland/rev/8676ddcded7b5e2079f5d399fb462bf2f9a3d3b8

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1287ef3bbba4
https://hg.mozilla.org/mozilla-central/rev/284f9a059a93

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Bobby, did you want to let this ride the 58 train?

Comment 43

[Manish Goregaokar [:manishearth]]

It's a memory improvement; I think it should. Has very little risk.

Bear in mind that https://hg.mozilla.org/integration/autoland/rev/8676ddcded7b5e2079f5d399fb462bf2f9a3d3b8 needs to land as well (hasn't hit m-c yet because it was hard to get a reviewer on saturday and I'm not sure what the r=orange policy is for servo when it comes to autoland oranges)

Comment 44

[Manish Goregaokar [:manishearth]]

Attachment: Uplift rev 1287ef3bbba4

Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]: Higher memory usage in Stylo
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Two more patches being uploaded
[Is the change risky?]: No
[Why is the change risky/not risky?]: Pure refactoring, should not affect behavior
[String changes made/needed]:

Comment 45

[Manish Goregaokar [:manishearth]]

Attachment: Uplift rev 284f9a059a93

(see previous approval request comment)

Comment 46

[Manish Goregaokar [:manishearth]]

Attachment: Uplift rev 8676ddcded7b

(see previous approval request comment)

This patch has not hit trunk/nightly yet, but should by tomorrow.

Comment 47

[Manish Goregaokar [:manishearth]]

(all three patches are r=emilio)

Comment 48

[Manish Goregaokar [:manishearth]]

Er, "I think it should" meant "I think it should be uplifted".

Comment 49

[Sylvestre Ledru [:Sylvestre]]

Could you give us an estimate of the memory gain? 
For now, I tend to reject the uplift to avoid potential regressions.

Comment 50

[Manish Goregaokar [:manishearth]]

One word per ComputedValues (~one word per non-sibling element on the page), plus a potential reduction of another word per ComputedValues based on the amount of sharing of nsStyleFont structs.

This is basically undoing a memory regression from bug 1380980 (+ adding more benefits due to style sharing); and shouldn't cause any behavior changes. We can let it bake on nightly for a while if you wish, but I really think we should uplift this for 57.

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Sorry but I am focusing on the product perspective, not on the implementation.
On a Facebook page, what do you think could be the memory gain? a few bytes, k, m?

Comment 52

[Manish Goregaokar [:manishearth]]

With a rough calculation and measurement I'm getting ~200kb.

Comment 53

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911625 [details] [diff] [review]
Uplift rev 1287ef3bbba4

Ok, significant enough that we should take it. Especially as it is rust code and early in the cycle.
Thanks.
Should be in 57b3

Comment 54

[Manish Goregaokar [:manishearth]]

Probably should wait one nightly cycle before the actual uplift though. Shouldn't break anything, but good to have that confirmation.

Comment 55

[Sylvestre Ledru [:Sylvestre]]

Next time, please wait until fill the uplift request then ;)

Comment 56

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #42)
> Bobby, did you want to let this ride the 58 train?

I could go either way. The memory savings are nice but we could live without them. Taking a large page increases risk, but there are some advantages to shipping/testing the same version of this font handling code on 57 and 58, and Manish says it simplifies things on the whole.

I told him to land it after the merge (which has happened), wait a few days to be sure there are no immediate regressions, and then let the release drivers make the call.

Comment 57

[Manish Goregaokar [:manishearth]]

Yeah, I intend to wait till at least tomorrow for getting this uplifted; thought I would preemptively request the review but it's probably best to wait and request when ready.

Comment 58

[Sylvestre Ledru [:Sylvestre]]

So, Manish, how do you feel about that?

Comment 59

[Manish Goregaokar [:manishearth]]

Attachment: Uplift rev 1287ef3bbba4

missed a patch

Comment 60

[Manish Goregaokar [:manishearth]]

Attachment: Uplift rev 05b0fccb403e

missed a patch

Comment 61

[Manish Goregaokar [:manishearth]]

Yeah, feels good. I however forgot to add the fourth patch when doing this the first time.

To be clear, the patches that need uplifting are (in order):


https://hg.mozilla.org/mozilla-central/rev/05b0fccb403e
https://hg.mozilla.org/mozilla-central/rev/1287ef3bbba4
https://hg.mozilla.org/mozilla-central/rev/284f9a059a93
https://hg.mozilla.org/mozilla-central/rev/8676ddcded7b

I've uploaded rebased versions of each one. IIRC the rebase has no conflicts, so it would be preferable to do this uplift directly instead of using the patches I uploaded.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=f58a47d4f7023717e2bb78d3c341347d448e4fab is the try push (sanity checking that I did the rebase correctly)

Comment 62

[Manish Goregaokar [:manishearth]]

Try build seems fine. re-approve?

Comment 63

[Sylvestre Ledru [:Sylvestre]]

Really sorry but I don't think I will take it for 57. We have a lot of changes and we have data which shows that 50% of the uplifts for performance/memory reasons will cause regressions.
Because 57 is a very special release, I would like to limit the number of potentially risky changes.
Sorry again.

Comment 64

[Bobby Holley (:bholley)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #63)
> Really sorry but I don't think I will take it for 57. We have a lot of
> changes and we have data which shows that 50% of the uplifts for
> performance/memory reasons will cause regressions.
> Because 57 is a very special release, I would like to limit the number of
> potentially risky changes.
> Sorry again.

No worries! Thanks for managing the risk, we can use our uplifts for crashes and compat issues and ship the memory improvements in 58.