Closed Bug 1399228 Opened 7 years ago Closed 7 years ago

stylo: Move font computation data to style structs

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(9 files, 1 obsolete file)

Bug 1399228 - Add keyword info to computed value of font-size;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1399228 - stylo: Add font keyword info fields on nsStyleFont;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1399228 - stylo: Remove FontComputationData, switch over to using the new font tracking;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1399228 - stylo: Move font-size stuff to values::*::font ;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1399228 - stylo: Clean up keyword values;
59 bytes, text/x-review-board-request
emilio
: review+
Details
Uplift rev 284f9a059a93
3.19 KB, patch
manishearth
: review+
Sylvestre
: approval-mozilla-beta-
Details | Diff | Splinter Review
Uplift rev 8676ddcded7b
3.58 KB, patch
manishearth
: review+
Sylvestre
: approval-mozilla-beta-
Details | Diff | Splinter Review
Uplift rev 1287ef3bbba4
2.84 KB, patch
Sylvestre
: approval-mozilla-beta-
Details | Diff | Splinter Review
Uplift rev 05b0fccb403e
95.69 KB, text/plain
Sylvestre
: approval-mozilla-beta-
Details 
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)
When do we compute them then? And how would you resolve em units?
P3, but still very much want this.
Priority: P2 → P3
> 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)
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 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.
Attachment #8908870 - Flags: review?(emilio) → review+
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.
Attachment #8908871 - Flags: review?(emilio) → review+
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.
Attachment #8908872 - Flags: review?(emilio) → review+
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 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.
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.
Flags: needinfo?(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.
Flags: needinfo?(manishearth)
(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 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.
Attachment #8909502 - Flags: review?(emilio) → review+
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?
Attachment #8909501 - Flags: review?(emilio) → review+
See Also: → 1401322
rebased, will land tomorrow
Pushed by manishearth@gmail.com:
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
https://github.com/servo/servo/pull/18609 is a fixup for fixing the orange in layout/style/test/test_transitions_per_property.html
Bobby, did you want to let this ride the 58 train?
Flags: needinfo?(bobbyholley)
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)
Attached patch Uplift rev 1287ef3bbba4 (obsolete) — — Splinter Review 
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]:
Attachment #8911625 - Flags: review+
Attachment #8911625 - Flags: approval-mozilla-beta?
(see previous approval request comment)
Attachment #8911627 - Flags: review+
Attachment #8911627 - Flags: approval-mozilla-beta?
(see previous approval request comment)

This patch has not hit trunk/nightly yet, but should by tomorrow.
Attachment #8911628 - Flags: review+
Attachment #8911628 - Flags: approval-mozilla-beta?
(all three patches are r=emilio)
Er, "I think it should" meant "I think it should be uplifted".
Could you give us an estimate of the memory gain? 
For now, I tend to reject the uplift to avoid potential regressions.
Flags: needinfo?(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.
Flags: needinfo?(manishearth)
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?
With a rough calculation and measurement I'm getting ~200kb.
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
Attachment #8911625 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8911627 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8911628 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Probably should wait one nightly cycle before the actual uplift though. Shouldn't break anything, but good to have that confirmation.
Attachment #8911625 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Attachment #8911627 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Attachment #8911628 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Next time, please wait until fill the uplift request then ;)
(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.
Flags: needinfo?(bobbyholley)
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.
So, Manish, how do you feel about that?
Flags: needinfo?(manishearth)
missed a patch
Attachment #8911625 - Attachment is obsolete: true
Attachment #8911625 - Flags: approval-mozilla-beta?
Flags: needinfo?(manishearth)
Attachment #8912606 - Flags: approval-mozilla-beta?
missed a patch
Attachment #8912607 - Flags: approval-mozilla-beta?
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)
Try build seems fine. re-approve?
Flags: needinfo?(sledru)
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.
Flags: needinfo?(sledru)
Attachment #8912607 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8912606 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8911628 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8911627 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: