Generate some PropertyDeclaration methods by hand 🐉🐲 #19956

nox · 2018-02-05T12:46:35Z

We rely on rust-lang/rfcs#2195 to make some enums share the same discriminants by definition.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1428285.

This change is


        Use less type aliases from longhands in PropertyDeclaration

This will help us reuse code between different variants in a later patch, to implement Clone by hand to optimise it.


        Use LonghandId::is_animatable in PropertyDeclaration::is_animatable

highfive · 2018-02-05T12:46:38Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/data.py, components/style/properties/helpers.mako.rs, components/style/properties/declaration_block.rs, components/style/properties/helpers/animated_properties.mako.rs, components/style/properties/properties.mako.rs
@canaltinova: components/style/properties/data.py, components/style/properties/helpers.mako.rs, components/style/properties/declaration_block.rs, components/style/properties/helpers/animated_properties.mako.rs, components/style/properties/properties.mako.rs
@emilio: components/style/properties/data.py, components/style/properties/helpers.mako.rs, components/style/properties/declaration_block.rs, components/style/properties/helpers/animated_properties.mako.rs, components/style/properties/properties.mako.rs

highfive · 2018-02-05T12:46:39Z

Warning

These commits modify unsafe code. Please review it carefully!

nox · 2018-02-05T12:53:18Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cccd012152be2f205e31bb00254900e85215152

SimonSapin · 2018-02-05T13:00:29Z

components/style/properties/properties.mako.rs

 /// Servo's representation for a property declaration.
 #[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
-#[derive(Clone, PartialEq)]
+#[derive(PartialEq)]
+#[repr(u8)]


Doesn’t this have more than 256 variants?

Yes. :D Just realised with the geckotry failing to build.

nox · 2018-02-05T13:08:02Z

I made the enum be #[repr(u16)].

https://treeherder.mozilla.org/#/jobs?repo=try&revision=680ae344ebda08a71b94be64049039d2328abd33

nox · 2018-02-05T23:02:21Z

[nox:~/src/gecko] 2s 130
⌀ size XUL.old
__TEXT	__DATA	__OBJC	others	dec	hex
88387584	5271552	0	65503232	159162368	97ca000

[nox:~/src/gecko]
⌀ ../bloaty/build/bloaty -d symbols -n 100000000 XUL.old | grep -w PropertyDeclaration
   0.0%  45.1Ki style::properties::PropertyDeclaration::to_css::h70aa9127e631f236                 45.1Ki   0.0%
   0.0%  32.8Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..cmp..PartialEq$GT$:  32.8Ki   0.0%
   0.0%  32.6Ki __ZN79_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..cmp..PartialE  32.6Ki   0.0%
   0.0%  29.7Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  29.7Ki   0.0%
   0.0%  29.3Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  29.3Ki   0.0%
   0.0%  27.9Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  27.9Ki   0.0%
   0.0%  27.6Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$GT$::c  27.6Ki   0.0%
   0.0%  15.7Ki __ZN87_$LT$style..properties..PropertyDeclaration$u20$as$u20$malloc_size_of..Mal  15.7Ki   0.0%
   0.0%  9.78Ki style::properties::PropertyDeclaration::parse_into::h35ee1deaf54c46b6             9.78Ki   0.0%
   0.0%  4.17Ki style::properties::PropertyDeclaration::id::h9f691ba9ad1fcbbc                     4.17Ki   0.0%
   0.0%     320 style::properties::PropertyDeclaration::get_system::h4a3d3afe5a217d86                320   0.0%
   0.0%      32 style::properties::PropertyDeclaration::get_css_wide_keyword::he3ca52066fa1abf7       32   0.0%

[nox:~/src/gecko] 2s
⌀ size XUL.new
__TEXT	__DATA	__OBJC	others	dec	hex
88190976	5271552	0	65482752	158945280	9795000

[nox:~/src/gecko]
⌀ ../bloaty/build/bloaty -d symbols -n 100000000 XUL.new | grep -w PropertyDeclaration
   0.0%  45.2Ki style::properties::PropertyDeclaration::to_css::hc9cb9df128aea03f                 45.2Ki   0.0%
   0.0%  22.8Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..cmp..PartialEq$GT$:  22.8Ki   0.0%
   0.0%  22.3Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$GT$::c  22.3Ki   0.0%
   0.0%  15.6Ki __ZN87_$LT$style..properties..PropertyDeclaration$u20$as$u20$malloc_size_of..Mal  15.6Ki   0.0%
   0.0%  9.89Ki style::properties::PropertyDeclaration::parse_into::ha8f87489fe4f89f8             9.89Ki   0.0%
   0.0%     320 style::properties::PropertyDeclaration::get_system::h07f9efc836477132                320   0.0%
   0.0%      96 style::properties::PropertyDeclaration::id::hd928f19c20f0e1bc                         96   0.0%
   0.0%      32 style::properties::PropertyDeclaration::get_css_wide_keyword::hfd74e2964361369d       32   0.0%

It looks good but I'm not sure I should trust those results.

Cc @bholley @emilio

Also, I've yet to make a geckotry with the latest commits I added.

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

nox · 2018-02-05T23:17:56Z

Silly me had not put #[inline] on the Clone and PartialEq methods, that explains why there was only one Clone in XUL.new:

⌀ ../bloaty/build/bloaty -d symbols -n 1000000000 XUL.new | grep -w PropertyDeclaration
   0.0%  45.2Ki style::properties::PropertyDeclaration::to_css::hc9cb9df128aea03f                 45.2Ki   0.0%
   0.0%  23.0Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..cmp..PartialEq$GT$:  23.0Ki   0.0%
   0.0%  22.7Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  22.7Ki   0.0%
   0.0%  22.7Ki __ZN79_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..cmp..PartialE  22.7Ki   0.0%
   0.0%  22.3Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  22.3Ki   0.0%
   0.0%  22.2Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  22.2Ki   0.0%
   0.0%  21.8Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$GT$::c  21.8Ki   0.0%
   0.0%  15.6Ki __ZN87_$LT$style..properties..PropertyDeclaration$u20$as$u20$malloc_size_of..Mal  15.6Ki   0.0%
   0.0%  9.89Ki style::properties::PropertyDeclaration::parse_into::ha8f87489fe4f89f8             9.89Ki   0.0%
   0.0%     320 style::properties::PropertyDeclaration::get_system::h07f9efc836477132                320   0.0%
   0.0%      96 style::properties::PropertyDeclaration::id::hd928f19c20f0e1bc                         96   0.0%
   0.0%      32 style::properties::PropertyDeclaration::get_css_wide_keyword::hfd74e2964361369d       32   0.0%

nox · 2018-02-05T23:24:57Z

Everything is orange. Will investigate tomorrow. Probably some dumb error somewhere.

nox · 2018-02-06T08:21:35Z

Latest results with bloaty, and the missing #[inline] on clone and eq:

[nox:~/src/gecko]
⌀ size XUL.old
__TEXT	__DATA	__OBJC	others	dec	hex
88387584	5271552	0	65503232	159162368	97ca000

[nox:~/src/gecko]
⌀ ../bloaty/build/bloaty -d symbols -n 1000000000 XUL.old | grep -w PropertyDeclaration
   0.0%  45.1Ki style::properties::PropertyDeclaration::to_css::h70aa9127e631f236                 45.1Ki   0.0%
   0.0%  32.8Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..cmp..PartialEq$GT$:  32.8Ki   0.0%
   0.0%  32.6Ki __ZN79_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..cmp..PartialE  32.6Ki   0.0%
   0.0%  29.7Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  29.7Ki   0.0%
   0.0%  29.3Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  29.3Ki   0.0%
   0.0%  27.9Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  27.9Ki   0.0%
   0.0%  27.6Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$GT$::c  27.6Ki   0.0%
   0.0%  15.7Ki __ZN87_$LT$style..properties..PropertyDeclaration$u20$as$u20$malloc_size_of..Mal  15.7Ki   0.0%
   0.0%  9.78Ki style::properties::PropertyDeclaration::parse_into::h35ee1deaf54c46b6             9.78Ki   0.0%
   0.0%  4.17Ki style::properties::PropertyDeclaration::id::h9f691ba9ad1fcbbc                     4.17Ki   0.0%
   0.0%     320 style::properties::PropertyDeclaration::get_system::h4a3d3afe5a217d86                320   0.0%
   0.0%      32 style::properties::PropertyDeclaration::get_css_wide_keyword::he3ca52066fa1abf7       32   0.0%

[nox:~/src/gecko] 3s
⌀ size XUL.new
__TEXT	__DATA	__OBJC	others	dec	hex
88317952	5271552	0	65495040	159084544	97b7000

[nox:~/src/gecko]
⌀ ../bloaty/build/bloaty -d symbols -n 1000000000 XUL.new | grep -w PropertyDeclaration
   0.0%  45.2Ki style::properties::PropertyDeclaration::to_css::he42a36e94b3deab2                 45.2Ki   0.0%
   0.0%  23.0Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..cmp..PartialEq$GT$:  23.0Ki   0.0%
   0.0%  22.7Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  22.7Ki   0.0%
   0.0%  22.7Ki __ZN79_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..cmp..PartialE  22.7Ki   0.0%
   0.0%  22.3Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  22.3Ki   0.0%
   0.0%  22.2Ki __ZN77_$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$  22.2Ki   0.0%
   0.0%  21.8Ki _$LT$style..properties..PropertyDeclaration$u20$as$u20$core..clone..Clone$GT$::c  21.8Ki   0.0%
   0.0%  15.6Ki __ZN87_$LT$style..properties..PropertyDeclaration$u20$as$u20$malloc_size_of..Mal  15.6Ki   0.0%
   0.0%  9.89Ki style::properties::PropertyDeclaration::parse_into::ha8f87489fe4f89f8             9.89Ki   0.0%
   0.0%     320 style::properties::PropertyDeclaration::get_system::h07f9efc836477132                320   0.0%
   0.0%      96 style::properties::PropertyDeclaration::id::hd928f19c20f0e1bc                         96   0.0%
   0.0%      32 style::properties::PropertyDeclaration::get_css_wide_keyword::hfd74e2964361369d       32   0.0%

Geckotry seems green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cb22ae5e6be97fcc968dc965827234d54f26b47&selectedJob=160528329

emilio

r=me, with the comments and a different name for is_size_type (feel free to rename it on a followup commit if it's easier).

emilio · 2018-02-06T09:38:35Z

components/style/properties/helpers.mako.rs

@@ -903,6 +903,7 @@
    <%call expr="longhand(name,
                          predefined_type=length_type,
                          logical=logical,
+                          is_size_type=True,


Let's rename this to something that screams out more, like is_gecko_size_type_hack or something.

emilio · 2018-02-06T09:38:35Z

components/style/properties/properties.mako.rs

-    %>
-    /// ${property.name}
-    ${property.camel_case}(${ty}),
+        variants.append({


Can you document why do we need this complexity here, please?

emilio · 2018-02-06T09:38:35Z

components/style/properties/properties.mako.rs

-        };
-        PropertyDeclarationId::Longhand(longhand_id)
+        PropertyDeclarationId::Longhand(
+            unsafe { *(self as *const _ as *const LonghandId) },


Can we debug-assert this somehow at least, to ensure nobody messes up? I guess if they mess up it'd be completely orange but...

Also, please add a comment about why this is ok here so people don't scream :).

emilio · 2018-02-06T09:38:35Z

components/style/properties/properties.mako.rs

+        match self.value.from_shorthand {
+            // Normally, we shouldn't be printing variables here if they came from
+            // shorthands. But we should allow properties that came from shorthand
+            // aliases. That also matches with the Gecko behavior.


Can you mention here that this is just a hack for -moz-transform, which is really a shorthand for who knows what reason?

Also we should try to unship that, so feel free to also add a FIXME(emilio) over there so I eventually do it.

emilio · 2018-02-06T09:38:35Z

components/style/properties/properties.mako.rs

+                ${" | ".join("{}(ref this)".format(v) for v in variants)} => {
+                    let other_repr =
+                        &*(other as *const _ as *const PropertyDeclarationVariantRepr<${ty}>);
+                    *this == other_repr.value


Wonderfully evil :(

emilio · 2018-02-06T09:38:35Z

components/style/properties/properties.mako.rs

@@ -267,7 +268,6 @@ pub mod animated_properties {
 %>

 /// Servo's representation for a property declaration.
-#[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
 #[repr(u16)]
 pub enum PropertyDeclaration {


Can you add a general overview of why don't we derive stuff here and similar?

emilio · 2018-02-06T09:41:22Z

If @SimonSapin sanity-checked this too it'd be nice, since he had concerns about doing the same kind of optimization in https://bugzilla.mozilla.org/show_bug.cgi?id=1428285#c2, it seems.


        Make all PropertyDeclaration variants have only one field


        Implement Clone for PropertyDeclaration by hand 🐉🐲

This makes the enum `#[repr(u16)]` and generate code that shares the same Clone::clone call for all variants sharing the same field type.

nox · 2018-02-06T12:57:44Z

I added a comment in the commit message.

@bors-servo r=emilio,SimonSapin

bors-servo · 2018-02-06T12:57:45Z

📌 Commit fd31712 has been approved by emilio,SimonSapin

bors-servo · 2018-02-06T13:02:58Z

⌛ Testing commit fd31712 with merge 5304e8f...


        Auto merge of #19956 - servo:derive-all-the-things, r=emilio,SimonSapin

Generate some PropertyDeclaration methods by hand 🐉🐲  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19956)

bors-servo · 2018-02-06T13:06:20Z

💔 Test failed - linux-rel-wpt


        Make LonghandId just be the discriminants of PropertyDeclaration 🐉🐲

This relies on the fact that `#[repr(u16)]` on enums ensure the discriminant will be the very first field of the enum value. This has always been the case, but it has been formally defined only since rust-lang/rfcs#2195. https://bugzilla.mozilla.org/show_bug.cgi?id=1428285


        Merge common arms in PropertyDeclaration::to_css


        Implement PartialEq for PropertyDeclaration by hand 🐉🐲

We first check that the discriminants are equal, then we pattern match only on `*self` and use `PropertyDeclarationVariantRepr` to look into `other` directly.


        Implement MallocSizeOf for PropertyDeclaration by hand

We merge arms with identical field types.

nox · 2018-02-06T13:12:29Z

@bors-servo r=emilio

bors-servo · 2018-02-06T13:12:30Z

📌 Commit 1f0a126 has been approved by emilio

bors-servo · 2018-02-06T13:12:35Z

⌛ Testing commit 1f0a126 with merge 9faf0cd...


        Auto merge of #19956 - servo:derive-all-the-things, r=emilio

Generate some PropertyDeclaration methods by hand 🐉🐲 We rely on rust-lang/rfcs#2195 to make some enums share the same discriminants by definition. This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1428285.  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19956)

bors-servo · 2018-02-06T14:05:54Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 9faf0cd to master...

ionutgoldan · 2018-02-13T06:47:23Z

These changes landed some fine performance improvements! Thank you!

== Change summary for alert #11504 (as of Tue, 06 Feb 2018 12:06:36 GMT) ==

Improvements:

5% Stylo Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench osx-10-10 opt 549,031.92 -> 519,840.33
2% Stylo Servo_DeclarationBlock_GetPropertyById_Bench windows7-32 opt 231,626.04 -> 226,308.08

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11504

bholley · 2018-02-13T15:31:38Z

There are some more wins in #19966 that might also be attributable to this.

Awesome job @nox!

nox added 2 commits Feb 4, 2018

Use less type aliases from longhands in PropertyDeclaration

90b37cf

This will help us reuse code between different variants in a later patch, to implement Clone by hand to optimise it.

Use LonghandId::is_animatable in PropertyDeclaration::is_animatable

3918c98

highfive assigned emilio Feb 5, 2018

highfive added the S-awaiting-review label Feb 5, 2018

SimonSapin reviewed Feb 5, 2018

View changes

nox force-pushed the derive-all-the-things branch from 9362314 to 2d59b50 Feb 5, 2018

nox force-pushed the derive-all-the-things branch 4 times, most recently from 516acf6 to eec63ec Feb 5, 2018

nox changed the title ~~Implement Clone for PropertyDeclaration by hand 🐉🐲~~ (Do not merge) Implement Clone for PropertyDeclaration by hand 🐉🐲 Feb 5, 2018

nox changed the title ~~(Do not merge) Implement Clone for PropertyDeclaration by hand 🐉🐲~~ (Do not merge) Generate some PropertyDeclaration methods 🐉🐲 Feb 5, 2018

nox force-pushed the derive-all-the-things branch from eec63ec to 2bdc118 Feb 5, 2018

nox force-pushed the derive-all-the-things branch 2 times, most recently from d4f22cb to 27fa1a7 Feb 5, 2018

nox changed the title ~~(Do not merge) Generate some PropertyDeclaration methods 🐉🐲~~ Generate some PropertyDeclaration methods 🐉🐲 Feb 6, 2018

nox changed the title ~~Generate some PropertyDeclaration methods 🐉🐲~~ Generate some PropertyDeclaration methods by hand 🐉🐲 Feb 6, 2018

emilio approved these changes Feb 6, 2018

View changes

nox force-pushed the derive-all-the-things branch from 4140a43 to 2ebdf21 Feb 6, 2018

nox added 2 commits Feb 5, 2018

Make all PropertyDeclaration variants have only one field

5195eb3

Implement Clone for PropertyDeclaration by hand 🐉🐲

7f98d8e

This makes the enum `#[repr(u16)]` and generate code that shares the same Clone::clone call for all variants sharing the same field type.

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 6, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 6, 2018

nox added 4 commits Feb 5, 2018

Merge common arms in PropertyDeclaration::to_css

8ebf0f0

Implement PartialEq for PropertyDeclaration by hand 🐉🐲

456c18f

We first check that the discriminants are equal, then we pattern match only on `*self` and use `PropertyDeclarationVariantRepr` to look into `other` directly.

Implement MallocSizeOf for PropertyDeclaration by hand

Loading status checks…

1f0a126

We merge arms with identical field types.

nox force-pushed the derive-all-the-things branch from fd31712 to 1f0a126 Feb 6, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Feb 6, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 6, 2018

highfive removed the S-awaiting-merge label Feb 6, 2018

nox deleted the derive-all-the-things branch Feb 6, 2018

bholley mentioned this pull request Feb 13, 2018

style: Get rid of gecko_size_type. #19966

Merged

nox mentioned this pull request Mar 16, 2018

Allow arbitrary enums to have explicit discriminants rust-lang/rfcs#2363

Merged

servo / servo

Generate some PropertyDeclaration methods by hand 🐉🐲 #19956

Generate some PropertyDeclaration methods by hand 🐉🐲 #19956

nox commented Feb 5, 2018 •

edited

highfive commented Feb 5, 2018

highfive commented Feb 5, 2018

nox commented Feb 5, 2018

This comment has been minimized.

This comment has been minimized.

nox commented Feb 5, 2018

nox commented Feb 5, 2018

nox commented Feb 5, 2018

nox commented Feb 5, 2018

nox commented Feb 6, 2018

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Feb 6, 2018

nox commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

nox commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

ionutgoldan commented Feb 13, 2018

bholley commented Feb 13, 2018

servo / servo

Join GitHub today

Generate some PropertyDeclaration methods by hand 🐉🐲 #19956

Generate some PropertyDeclaration methods by hand 🐉🐲 #19956

Conversation

nox commented Feb 5, 2018 • edited

highfive commented Feb 5, 2018

highfive commented Feb 5, 2018

nox commented Feb 5, 2018

This comment has been minimized.

This comment has been minimized.

nox commented Feb 5, 2018

nox commented Feb 5, 2018

nox commented Feb 5, 2018

nox commented Feb 5, 2018

nox commented Feb 6, 2018

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Feb 6, 2018

nox commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

nox commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

bors-servo commented Feb 6, 2018

ionutgoldan commented Feb 13, 2018

bholley commented Feb 13, 2018

nox commented Feb 5, 2018 •

edited