Closed Bug 1397626 Opened 7 years ago Closed 7 years ago

stylo: Consider creating refcounted wrapper object for FontFamilyList and store it in specified value so that it can be shared between style structs

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: xidorn, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1397626 - Part 1: Add a SharedFontList class.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1397626 - Part 2: Replace uses of FontFamilyListRefCnt with SharedFontList.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1397626 - Part 3: Use SharedFontList to store font-family specified and computed values.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Because of insufficient sharing of font struct, it seems that array buffer for family name list shows up to be a significant part in memory usage (see bug 1397380 comment 8).

We may want to use a refcounted wrapper around FontFamilyList, and store a pointer to it in specified value, so that it can be shared between style structs.
Blocks: 1367854
Priority: -- → P3
Assignee: nobody → cam
Status: NEW → ASSIGNED
I have a WIP patch that adds a refcounted wrapper to FontFamilyList::mFontlist (the nsTArray), since that ended up being easier than making the FontFamilyList itself refcounted, just due to the generic fixups we do after computing font-family.

On the HTML spec, I get ~9000 FontFamilyList objects, and with the patch, only 419 unique refcounted mFontlist objects.  With some instrumenting it looks like this will save around 730 KB.
Comment on attachment 8913611 [details]
style: Use a SharedFontlist object to store font-family values for Gecko.

https://reviewboard.mozilla.org/r/184996/#review190182

::: servo/components/style/properties/longhand/font.mako.rs:386
(Diff revision 1)
> +                FontFamilyList(families)
> +            }
> +
> +            #[cfg(feature = "gecko")]
> +            pub fn new(families: Vec<FontFamily>) -> FontFamilyList {
> +                let mut names: structs::nsTArray<structs::FontFamilyName>;

I would be much more comfortable if this wasn't on the stack from the start.

Can we instead have the Construct call construct a RefPtr for us?

::: servo/components/style/properties/longhand/font.mako.rs:389
(Diff revision 1)
> +            #[cfg(feature = "gecko")]
> +            pub fn new(families: Vec<FontFamily>) -> FontFamilyList {
> +                let mut names: structs::nsTArray<structs::FontFamilyName>;
> +                unsafe {
> +                    names = mem::uninitialized();
> +                    bindings::Gecko_nsTArray_FontFamilyName_Construct(&mut names);

It's best if the construct call uses its knowledge of the length of the vec to preallocate.

It also means we can do the AppendFoo foo stuff in pure Rust; if the array is preallocated with its length already set to `families.len()`. But this isn't necessary.

::: servo/components/style/properties/longhand/font.mako.rs:417
(Diff revision 1)
> +                        }
> +                    }
> +                }
> +
> +                unsafe {
> +                    let ptr = bindings::Gecko_SharedFontlist_Create(&mut names);

I don't really like this "move" here, but this will go away once we create the `RefPtr<SharedFontList>` off the bat and write to it directly

::: servo/components/style/properties/longhand/font.mako.rs:2364
(Diff revision 1)
>                          cx.style().get_font().gecko(),
>                          cx.device().pres_context()
>                      )
>                  }
> -                let family = system.fontlist.mFontlist.iter().map(|font| {
> +                let names = unsafe { &system.fontlist.mFontlist.mRawPtr.as_ref().unwrap().mNames };
> +                let family = names.iter().map(|font| {

can't we just addref this in gecko mode?
Attachment #8913611 - Flags: review?(manishearth) → review+
Comment on attachment 8913611 [details]
style: Use a SharedFontlist object to store font-family values for Gecko.

https://reviewboard.mozilla.org/r/184996/#review190182

> I would be much more comfortable if this wasn't on the stack from the start.
> 
> Can we instead have the Construct call construct a RefPtr for us?

The model I chose on the C++ side was that a SharedFontlist object is immutable, and constructed with an array of FontFamilyName objects that it steals the contents of.  (So that we don't have the ability to change the object when it's being shared.)  I could have some other way to build up list of FontFamilyNames to then pass to Gecko_SharedFontlist_Create, e.g. just have a Vec, then I call an FFI function to in-place construct each element (since they're non-POD), and then pass a pointer/length, and the SharedFontlist clones them all.  But I think it's slightly more straightforward to just use the nsTArray.

> It's best if the construct call uses its knowledge of the length of the vec to preallocate.
> 
> It also means we can do the AppendFoo foo stuff in pure Rust; if the array is preallocated with its length already set to `families.len()`. But this isn't necessary.

But can we do the AppendFoo things from Rust?  I didn't think we could construct a new nsString from the Rust side.

> can't we just addref this in gecko mode?

Yes, good point!
Comment on attachment 8913611 [details]
style: Use a SharedFontlist object to store font-family values for Gecko.

https://reviewboard.mozilla.org/r/184996/#review190276

::: servo/components/style/properties/longhand/font.mako.rs:387
(Diff revision 1)
> +                unsafe {
> +                    names = mem::uninitialized();
> +                    bindings::Gecko_nsTArray_FontFamilyName_Construct(&mut names);
> +                }

I don't really like this either.

Since empty `nsTArray`s are basically just a pointer to `nsTArrayHeader::sEmptyHdr`, I think you can create `new()` method for `nsTArray` in gecko_bindings/sugar. That way we would not need this `unsafe` block anymore, as well as the `Gecko_nsTArray_FontFamilyName_Construct` FFI function.

::: servo/components/style/properties/longhand/font.mako.rs:418
(Diff revision 1)
> +                    }
> +                }
> +
> +                unsafe {
> +                    let ptr = bindings::Gecko_SharedFontlist_Create(&mut names);
> +                    bindings::Gecko_nsTArray_FontFamilyName_Destroy(&mut names);

I don't really like this function either... Probably we can add `Drop` impl for `nsTArray` in sugar, and `assert!` (or at least `debug_assert!`) that it is referring to `sEmptyHdr` so that we don't leak anything.
Comment on attachment 8913610 [details]
Bug 1397626 - Part 1: Add a SharedFontList class.

https://reviewboard.mozilla.org/r/184994/#review190262

In general, I think the idea is good. There are some non-trivial changes I'd like to see, so cancel r? for now.

::: gfx/thebes/gfxFontFamilyList.h:188
(Diff revision 1)
> +    explicit SharedFontlist(const nsAString& aFamilyName,
> +                            QuotedName aQuoted)

I don't think you need `explicit` here... Gecko's lint only require `explicit` for constructors which can be used with a single parameter. I guess it isn't harmful either, though.

::: gfx/thebes/gfxFontFamilyList.h:200
(Diff revision 1)
> +        : mNames { aName }
> +    {
> +    }
> +
> +    explicit SharedFontlist(nsTArray<FontFamilyName>&& aNames)
> +        : mNames(aNames)

You need `Move(aNames)` here, otherwise you would be calling copy constructor.

::: gfx/thebes/gfxFontFamilyList.h:204
(Diff revision 1)
> +    size_t SizeOfIncludingThisIfUnshared(MallocSizeOf aMallocSizeOf) const
> +    {
> +        size_t n = 0;
> +        if (mRefCnt.get() == 1) {
> +          n += aMallocSizeOf(this);
> +          n += mNames.ShallowSizeOfExcludingThis(aMallocSizeOf);
> +          for (const FontFamilyName& name : mNames) {
> +              n += name.SizeOfExcludingThis(aMallocSizeOf);
> +          }
> +        }
> +        return n;
> +    }

IIUC, this is shared between specified value and computed value for Servo backend, and it is never shared for Gecko backend?

In that case, I guess we can just add `SizeOfIncludingThis`, and call it in specified value for Servo, and computed value for Gecko?

Oh, hmmm, checking backend is probably hard for measurement of computed value. Probably both `SizeOfIncludingThis` and `SizeOfIncludingThisIfUnshared`, and call the former for specified value in Servo, and latter for computed value?

My main concern is that most of font list which is used would be shared between computed value and specified value, so that could become a significant part of unreported memory usage for Stylo.

::: gfx/thebes/gfxFontFamilyList.h:231
(Diff revision 1)
> +
> +/**
>   * font family list, array of font families and a default font type.
>   * font family names are either named strings or generics. the default
>   * font type is used to preserve the variable font fallback behavior
>   */ 

As far as you are here, probably consider remove the trailing space of this line.

::: gfx/thebes/gfxFontFamilyList.h:333
(Diff revision 1)
> -                    mFontlist.RemoveElementAt(i);
> -                    mFontlist.InsertElementAt(0, name);
> +                    nsTArray<FontFamilyName> names;
> +                    names.AppendElements(mFontlist->mNames);
> +                    names.RemoveElementAt(i);
> +                    names.InsertElementAt(0, name);
> +                    SetFontlist(Move(names));

Hmmm, this feels a bit painful for users who don't use document fonts... But to change this situation, we may need some non-trivial refactor... I guess it is fine for now :/

::: layout/style/ServoBindings.h:297
(Diff revision 1)
> +void Gecko_nsTArray_FontFamilyName_Construct(nsTArray<FontFamilyName>* names);
> +void Gecko_nsTArray_FontFamilyName_AppendNamed(nsTArray<FontFamilyName>* names, nsIAtom* name, bool quoted);
> +void Gecko_nsTArray_FontFamilyName_AppendGeneric(nsTArray<FontFamilyName>* names, FontFamilyType type);
> +void Gecko_nsTArray_FontFamilyName_Destroy(nsTArray<FontFamilyName>* names);
> +void Gecko_FontFamilyList_SetFontlist(FontFamilyList* list, nsTArray<FontFamilyName>* names);
> +SharedFontlist* Gecko_SharedFontlist_Create(nsTArray<FontFamilyName>* names);

Please comment here whether the returned value is addrefed or not. Also it seems you are moving the contents from `names`. You should probably document that in comment as well.

::: layout/style/ServoBindings.cpp:1259
(Diff revision 1)
>  void
> -Gecko_FontFamilyList_Clear(FontFamilyList* aList) {
> -  aList->Clear();
> +Gecko_nsTArray_FontFamilyName_Construct(nsTArray<FontFamilyName>* aNames)
> +{
> +  new (aNames) nsTArray<FontFamilyName>();
> +}
> +
> +void
> +Gecko_nsTArray_FontFamilyName_Destroy(nsTArray<FontFamilyName>* aNames)
> +{
> +  aNames->~nsTArray();
>  }

Conceptually I don't really like these two methods...

It would be great if we can remove them. See my comment 7.

::: layout/style/ServoBindings.cpp:1292
(Diff revision 1)
>  void
> +Gecko_FontFamilyList_SetFontlist(FontFamilyList* aList,
> +                                 nsTArray<FontFamilyName>* aNames)
> +{
> +  aList->SetFontlist(Move(*aNames));
> +}

I don't see you're using this function in the second part?

::: layout/style/nsCSSValue.h:339
(Diff revision 1)
>  class FontFamilyListRefCnt final : public FontFamilyList {
>  public:
> -    FontFamilyListRefCnt()
> -        : FontFamilyList()
> +    FontFamilyListRefCnt(nsTArray<FontFamilyName>&& aNames)
> +        : FontFamilyList(Move(aNames))
> -    {}
> -
> -    explicit FontFamilyListRefCnt(FontFamilyType aGenericType)
> -        : FontFamilyList(aGenericType)
> -    {}
> -
> -    FontFamilyListRefCnt(const nsAString& aFamilyName,
> -                         QuotedName aQuoted)
> -        : FontFamilyList(aFamilyName, aQuoted)
> -    {}
> -
> -    FontFamilyListRefCnt(const FontFamilyListRefCnt& aOther)
> -        : FontFamilyList(aOther)
>      {}
>  
>      NS_INLINE_DECL_REFCOUNTING(FontFamilyListRefCnt);
>  
>  private:
>      ~FontFamilyListRefCnt() {}
>  };

It... seems to me that we should just replace `FontFamilyListRefCnt` with the new `SharedFontList` everywhere... I don't see how the extra `mDefaultFontType` field makes any sense for any of the users of `FontFamilyListRefCnt`...

Could you add a patch before this one for creating `SharedFontList` and use it to replace `FontFamilyListRefCnt`?
Attachment #8913610 - Flags: review?(xidorn+moz)
(In reply to Manish Goregaokar [:manishearth] from comment #5)
> Can we instead have the Construct call construct a RefPtr for us?

I decided to do this in the end and made some other changes, can you re-review?
Flags: needinfo?(manishearth)
Comment on attachment 8913610 [details]
Bug 1397626 - Part 1: Add a SharedFontList class.

https://reviewboard.mozilla.org/r/184994/#review190402
Attachment #8913610 - Flags: review?(xidorn+moz) → review+
changes look good to me
Flags: needinfo?(manishearth)
Comment on attachment 8913610 [details]
Bug 1397626 - Part 1: Add a SharedFontList class.

https://reviewboard.mozilla.org/r/184994/#review190824

::: gfx/thebes/gfxFontFamilyList.h:174
(Diff revision 3)
>  /**
> + * A refcounted array of FontFamilyNames.  We use this to store the specified
> + * value (in Servo) and the computed value (in both Gecko and Servo) of the
> + * font-family property.
> + */
> +class SharedFontlist

Probably `SharedFontList` (uppercase `L`) instead? It seems to me although there are variables called `fontlist`, types are consistently using `FontList`.

Probably even better `SharedFontFamilyList` so that people wouldn't get confused between this and the platform font lists.
Comment on attachment 8913610 [details]
Bug 1397626 - Part 1: Add a SharedFontList class.

https://reviewboard.mozilla.org/r/184994/#review190824

> Probably `SharedFontList` (uppercase `L`) instead? It seems to me although there are variables called `fontlist`, types are consistently using `FontList`.
> 
> Probably even better `SharedFontFamilyList` so that people wouldn't get confused between this and the platform font lists.

If you don't want it to be that long, probably `SharedFamilyList` is also fine?
Comment on attachment 8914160 [details]
Bug 1397626 - Part 2: Replace uses of FontFamilyListRefCnt with SharedFontList.

https://reviewboard.mozilla.org/r/185488/#review190822

::: layout/style/nsCSSParser.cpp:4042
(Diff revision 2)
>      REPORT_UNEXPECTED_TOKEN(PEFFVNoFamily);
>      return false;
>    }
>  
>    // add family to rule
> -  const FontFamilyList* fontlist = fontlistValue.GetFontFamilyListValue();
> +  SharedFontlist* fontlist = fontlistValue.GetFontFamilyListValue();

It seems to me that we could keep the `const` here?

::: layout/style/nsCSSRules.h:193
(Diff revision 2)
> -  const mozilla::FontFamilyList& GetFamilyList() { return mFamilyList; }
> -  void SetFamilyList(const mozilla::FontFamilyList& aFamilyList);
> +  mozilla::SharedFontlist* GetFontlist() const { return mFontlist; }
> +  void SetFontlist(mozilla::SharedFontlist* aFontlist) { mFontlist = aFontlist; }

With comment for part 1, it is probably better keep `family` in the name so that people wouldn't get confused. And it is probably better uppercase the `L`.
Attachment #8914160 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8913610 [details]
Bug 1397626 - Part 1: Add a SharedFontList class.

https://reviewboard.mozilla.org/r/184994/#review190824

> If you don't want it to be that long, probably `SharedFamilyList` is also fine?

Hmmm... It could be confused with `FontFamilyList` then... naming is hard :/

Probably fine just keep the current name...
Comment on attachment 8914161 [details]
Bug 1397626 - Part 3: Use SharedFontList to store font-family specified and computed values.

https://reviewboard.mozilla.org/r/185490/#review190830

::: gfx/thebes/gfxFontFamilyList.h:170
(Diff revision 2)
> - * A refcounted array of FontFamilyNames.
> + * A refcounted array of FontFamilyNames.  We use this to store the specified
> + * value (in Servo) and the computed value (in both Gecko and Servo) of the
> + * font-family property.

Why is the additional comment removed in part 2 but added again in part 3? I guess you shouldn't add that part back.

::: gfx/thebes/gfxFontFamilyList.h:438
(Diff revision 2)
>      size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const {
>          return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
>      }
>  
> -private:
> -    nsTArray<FontFamilyName>   mFontlist;
> +protected:
> +    RefPtr<SharedFontlist>     mFontlist;

Maybe consider wrapping this with `NotNull` so that people know that this would never be null.

::: layout/style/nsRuleNode.cpp:4379
(Diff revision 2)
>    uint8_t generic = kGenericFont_NONE;
>    // XXXldb What if we would have had a string if we hadn't been doing
>    // the optimization with a non-null aStartStruct?
>    const nsCSSValue* familyValue = aRuleData->ValueForFontFamily();
>    if (eCSSUnit_FontFamilyList == familyValue->GetUnit()) {
> -    const SharedFontlist* fontlist = familyValue->GetFontFamilyListValue();
> +    SharedFontlist* fontlist = familyValue->GetFontFamilyListValue();

Why do you need to remove `const` here?
Attachment #8914161 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8914160 [details]
Bug 1397626 - Part 2: Replace uses of FontFamilyListRefCnt with SharedFontList.

https://reviewboard.mozilla.org/r/185488/#review190822

> It seems to me that we could keep the `const` here?

I need to AddRef the object (in nsCSSFontFeatureValuesRule::SetFontlist), so it can't be const.
Comment on attachment 8914161 [details]
Bug 1397626 - Part 3: Use SharedFontList to store font-family specified and computed values.

https://reviewboard.mozilla.org/r/185490/#review190830

> Why is the additional comment removed in part 2 but added again in part 3? I guess you shouldn't add that part back.

Oh, I meant to remove the additional comment from part 1, and only add it in part 3.
Attachment #8913611 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb0291d4577f
Part 1: Add a SharedFontList class. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/16c2166700f2
Part 2: Replace uses of FontFamilyListRefCnt with SharedFontList. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/d49cf4aed335
Part 3: Use SharedFontList to store font-family specified and computed values. r=xidorn
Can this ride the 58 train?
Flags: needinfo?(cam)
I don't think we should uplift this.
Sold!
status-firefox57: affected → wontfix
Flags: needinfo?(cam)
Agreed.  (Assuming you meant the 57 train.)
Depends on: 1434802
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: