stylo: Move font computation data to style structs

RESOLVED FIXED in Firefox 58

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
3.19 KB, patch
manishearth
: review+
Details | Diff | Splinter Review
3.58 KB, patch
manishearth
: review+
Details | Diff | Splinter Review
2.84 KB, patch
Details | Diff | Splinter Review
95.69 KB, text/plain
Details
(Assignee)

Description

3 months ago
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
(Assignee)

Comment 3

3 months ago
> 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)
status-firefox55: --- → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox-esr52: --- → wontfix
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

3 months ago
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

3 months ago
mozreview-review
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 9

3 months ago
mozreview-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 10

3 months ago
mozreview-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+
(Assignee)

Comment 11

3 months ago
mozreview-review-reply
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)
(Assignee)

Comment 12

3 months ago
mozreview-review-reply
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)
(Assignee)

Comment 14

3 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

3 months ago
mozreview-review
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 28

3 months ago
mozreview-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+
(Assignee)

Updated

3 months ago
See Also: → bug 1401322
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 36

3 months ago
rebased, will land tomorrow
(Assignee)

Comment 37

3 months ago
https://github.com/servo/servo/pull/18607

Comment 38

3 months ago
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
(Assignee)

Comment 39

3 months ago
https://github.com/servo/servo/pull/18609 is a fixup for fixing the orange in layout/style/test/test_transitions_per_property.html
(Assignee)

Comment 40

3 months ago
https://hg.mozilla.org/integration/autoland/rev/8676ddcded7b5e2079f5d399fb462bf2f9a3d3b8
https://hg.mozilla.org/mozilla-central/rev/1287ef3bbba4
https://hg.mozilla.org/mozilla-central/rev/284f9a059a93
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Bobby, did you want to let this ride the 58 train?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 43

3 months ago
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)
(Assignee)

Comment 44

3 months ago
Created attachment 8911625 [details] [diff] [review]
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]:
Attachment #8911625 - Flags: review+
Attachment #8911625 - Flags: approval-mozilla-beta?
(Assignee)

Comment 45

3 months ago
Created attachment 8911627 [details] [diff] [review]
Uplift rev 284f9a059a93

(see previous approval request comment)
Attachment #8911627 - Flags: review+
Attachment #8911627 - Flags: approval-mozilla-beta?
(Assignee)

Comment 46

3 months ago
Created attachment 8911628 [details] [diff] [review]
Uplift rev 8676ddcded7b

(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?
(Assignee)

Comment 47

3 months ago
(all three patches are r=emilio)
(Assignee)

Comment 48

3 months ago
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)
(Assignee)

Comment 50

3 months ago
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?
(Assignee)

Comment 52

3 months ago
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+
(Assignee)

Comment 54

3 months ago
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)
(Assignee)

Comment 57

3 months ago
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)
(Assignee)

Comment 59

3 months ago
Created attachment 8912606 [details] [diff] [review]
Uplift rev 1287ef3bbba4

missed a patch
Attachment #8911625 - Attachment is obsolete: true
Attachment #8911625 - Flags: approval-mozilla-beta?
Flags: needinfo?(manishearth)
Attachment #8912606 - Flags: approval-mozilla-beta?
(Assignee)

Comment 60

3 months ago
Created attachment 8912607 [details]
Uplift rev 05b0fccb403e

missed a patch
Attachment #8912607 - Flags: approval-mozilla-beta?
(Assignee)

Comment 61

3 months ago
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)
(Assignee)

Comment 62

3 months ago
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.
status-firefox57: affected → wontfix
You need to log in before you can comment on or make changes to this bug.