1261552 - Implement support for placement-constructing, copy-constructing, and destroying Gecko style structs from Servo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

I have patches. Need to run them through try.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81214b74b135

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=016ba3c71d06

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Reimplement default placement-new for style structs. v1

We require a pointer of the class type, rather than void*, to reduce
the risk of accidentally calling this overload instead of the PresContext
one.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Introduce StaticPresData and hoist some shared functionality into it. v1

The complexity around the font pref cache stuff is really annoying. If we
think it's unnecessary, we could remove it in a followup.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Introduce StyleStructContext, and make all style struct constructors take it. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add FFI hooks to construct, copy, and destroy gecko style structs from servo. v1

Comment 7

[Cameron McCormack (:heycam)]

Note for the future: since structs are allocated from a pres arena, they're poisoned when they're destroyed.  I wonder if we should do the same for Servo-allocated structs, or if that doesn't make sense if Servo isn't using an arena like this?

(Will review patches properly tomorrow.)

Comment 8

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #7)
> Note for the future: since structs are allocated from a pres arena, they're
> poisoned when they're destroyed.  I wonder if we should do the same for
> Servo-allocated structs, or if that doesn't make sense if Servo isn't using
> an arena like this?

I don't think it makes any more sense to poison servo-allocated style structs than to poison ServoStyleSheet or ServoComputedValues.

My general sense is that style-struct poisoning was necessary when both style structs and rule nodes had complicated ownership semantics (the former through ancestor-based sharing, the latter through the GC). In the new world, we have a const RefPtr<ServoComputedValues> on the nsStyleContext, and the style structs are just pointers into data owned by the ServoComputedValues (we could refcount them if we wanted, but probably don't need to). Moreover, Gecko style structs were always expected to disappear completely when the relevant PresContext was destroyed, but that isn't the case for servo (where style structs can be shared among documents).

It's hard for me to see how poisoning would help us enough here to justify the complexity of implementing it on the servo side.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8737480 [details] [diff] [review]
Part 2 - Introduce StaticPresData and hoist some shared functionality into it. v1

Review of attachment 8737480 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/StaticPresData.h
@@ +89,5 @@
> +   * to actual nscoord values.
> +   */
> +  const nscoord* GetBorderWidthTable() { return mBorderWidthTable; }
> +
> +  // These are private, use the list in nsFont.h if you want a public list.

Can you move this enum into StaticPresData.cpp?

@@ +115,5 @@
> +   * be fine. But just to be on the safe side, we leave the old mechanism as-is,
> +   * with an additional per-session cache that new callers can use if they don't
> +   * have a PresContext.
> +   */
> +  const LangGroupFontPrefs* GetFontPrefsForLangShared(nsIAtom* aLanguage,

I am confused by the name of this method (and the following one), since it includes "Shared" but does not operate on the shared mStaticLangGroupFontPrefs data.  What is shared about it?

@@ +144,5 @@
> +  /*
> +   * These versions operate on the font pref cache on StaticPresData.
> +   */
> +
> +  const nsFont* GetDefaultFont(uint8_t aFontID, nsIAtom *aLanguage) const

Nit: "*" next to type.

@@ +157,5 @@
> +  }
> +
> +  void ResetCachedFontPrefs() { mStaticLangGroupFontPrefs.Reset(); }
> +
> +

Nit: one blank line is probably sufficient.

@@ +167,5 @@
> +  nscoord mBorderWidthTable[3];
> +  LangGroupFontPrefs mStaticLangGroupFontPrefs;
> +};
> +
> +

Nit: here too.

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8737481 [details] [diff] [review]
Part 3 - Introduce StyleStructContext, and make all style struct constructors take it. v1

Review of attachment 8737481 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/StyleStructContext.h
@@ +61,5 @@
> +    // font pref cache gets used. The distinction is probably unnecessary.
> +    SERVO_DEFAULT(mozilla::StaticPresData::Get()->
> +                  GetDefaultFont(aFontID, GetLanguageFromCharset()));
> +    return mPresContext->GetDefaultFont(aFontID, GetLanguageFromCharset());
> +

Nit: remove blank line.

::: layout/style/nsStyleContext.cpp
@@ +510,5 @@
>        result = new (presContext) nsStyle##c_(__VA_ARGS__); \
>        break;
>  
>    UNIQUE_CASE(Border, presContext)
> +  UNIQUE_CASE(Padding, presContext)

May as well just move the presContext argument up into the UNIQUE_CASE definition and get rid of the __VA_ARGS__ stuff.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8737482 [details] [diff] [review]
Part 4 - Add FFI hooks to construct, copy, and destroy gecko style structs from servo. v1

Review of attachment 8737482 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoBindings.cpp
@@ +152,5 @@
>  }
>  
> +#define STYLE_STRUCT_INHERITED(name, checkdata_cb) \
> +void \
> +Gecko_Construct_nsStyle##name(nsStyle##name* ptr) { new (ptr) nsStyle##name(StyleStructContext::ServoContext()); } \

This would be easier to read with line breaks in the usual places, e.g.

void                                                                \
Gecko_Construct_nsStyle##name(nsStyle##name* ptr)                   \
{                                                                   \
  new (ptr) nsStyle##name(StyleStructContext::ServoContext());      \
}

so please do that for these three methods.

::: layout/style/ServoBindings.h
@@ +85,5 @@
>  // Servo API.
>  void Servo_RestyleDocument(RawGeckoDocument* doc, RawServoStyleSet* set);
>  
> +// Style-struct management.
> +#define STYLE_STRUCT_INHERITED(name, checkdata_cb) \

Just define STYLE_STRUCT rather than STYLE_STRUCT_INHERITED and STYLE_STRUCT_RESET (and in ServoBindings.cpp).

Comment 12

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #9)
> Can you move this enum into StaticPresData.cpp?

Done.

> I am confused by the name of this method (and the following one), since it
> includes "Shared" but does not operate on the shared
> mStaticLangGroupFontPrefs data.  What is shared about it?

Shared in the "this is helper code shared by StaticPresData and PresContext". I've renamed "Shared" to "Helper", which is presumably less ambiguous.

> 
> @@ +144,5 @@
> > +  /*
> > +   * These versions operate on the font pref cache on StaticPresData.
> > +   */
> > +
> > +  const nsFont* GetDefaultFont(uint8_t aFontID, nsIAtom *aLanguage) const
> 
> Nit: "*" next to type.
> 
> @@ +157,5 @@
> > +  }
> > +
> > +  void ResetCachedFontPrefs() { mStaticLangGroupFontPrefs.Reset(); }
> > +
> > +
> 
> Nit: one blank line is probably sufficient.
> 
> @@ +167,5 @@
> > +  nscoord mBorderWidthTable[3];
> > +  LangGroupFontPrefs mStaticLangGroupFontPrefs;
> > +};
> > +
> > +
> 
> Nit: here too.

Fixed.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c202b18fa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/74a65e4f87f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/767a3ddd89e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c185e1c3be05

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3c202b18fa2
https://hg.mozilla.org/mozilla-central/rev/74a65e4f87f0
https://hg.mozilla.org/mozilla-central/rev/767a3ddd89e4
https://hg.mozilla.org/mozilla-central/rev/c185e1c3be05

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

hey, this made the a11y talos test show an improvement:
https://treeherder.mozilla.org/perf.html#/alerts?id=752

thanks for making Firefox faster:)