If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 48

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(4 attachments)

I have patches. Need to run them through try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=81214b74b135
https://treeherder.mozilla.org/#/jobs?repo=try&revision=016ba3c71d06
Created attachment 8737479 [details] [diff] [review]
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.
Attachment #8737479 - Flags: review?(cam)
Created attachment 8737480 [details] [diff] [review]
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.
Attachment #8737480 - Flags: review?(cam)
Created attachment 8737481 [details] [diff] [review]
Part 3 - Introduce StyleStructContext, and make all style struct constructors take it. v1
Attachment #8737481 - Flags: review?(cam)
Created attachment 8737482 [details] [diff] [review]
Part 4 - Add FFI hooks to construct, copy, and destroy gecko style structs from servo. v1
Attachment #8737482 - Flags: review?(cam)
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.)
(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.
Attachment #8737479 - Flags: review?(cam) → review+
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.
Attachment #8737480 - Flags: review?(cam) → review+
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.
Attachment #8737481 - Flags: review?(cam) → review+
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).
Attachment #8737482 - Flags: review?(cam) → review+
(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

2 years ago
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

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
hey, this made the a11y talos test show an improvement:
https://treeherder.mozilla.org/perf.html#/alerts?id=752

thanks for making Firefox faster:)
You need to log in before you can comment on or make changes to this bug.