Avoid debug serialization of PropertyDeclaration & co in release builds #19779

bholley · 2018-01-15T23:46:41Z

In total, this PR saves ~60k.

The conditional compilation on the _Debug FFI function eliminates one
of the ToCss variants, eliminating 54.4k, as well as a bunch of other
<1k functions. Removing the public trait implementation of Debug for the
font metrics provider eliminates the last Debug impl from stylo.

This change is

highfive · 2018-01-15T23:46:44Z

Heads up! This PR modifies the following files:

@canaltinova: components/style/font_metrics.rs
@emilio: ports/geckolib/glue.rs, components/style/font_metrics.rs

highfive · 2018-01-15T23:46:46Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

emilio

LGTM, that was an easy win. I bet way easier than finding it :)

emilio · 2018-01-15T23:50:34Z

@bors-servo r+

bors-servo · 2018-01-15T23:50:35Z

📌 Commit 903cf60 has been approved by emilio

bors-servo · 2018-01-15T23:50:39Z

⌛ Testing commit 903cf60 with merge 3fb9f71...

Avoid debug serialization of PropertyDeclaration & co in release builds In total, this PR saves ~60k. The conditional compilation on the _Debug FFI function eliminates one of the ToCss variants, eliminating 54.4k, as well as a bunch of other <1k functions. Removing the public trait implementation of Debug for the font metrics provider eliminates the last Debug impl from stylo.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19779)

bors-servo · 2018-01-16T00:24:54Z

💔 Test failed - linux-rel-css

jdm · 2018-01-16T00:49:47Z

error[E0425]: cannot find value `Servo_StyleRule_Debug` in this scope
  --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:88:7
   |
88 |     [ Servo_StyleRule_Debug, bindings::Servo_StyleRule_Debug ];
   |       ^^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_StyleRule_AddRef`?
   |
help: possible candidate is found in another module, you can import it into scope
   |
7  | use style::gecko_bindings::bindings::Servo_StyleRule_Debug;
   |

error[E0425]: cannot find value `Servo_ImportRule_Debug` in this scope
  --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:91:7
   |
91 |     [ Servo_ImportRule_Debug, bindings::Servo_ImportRule_Debug ];
   |       ^^^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_ImportRule_AddRef`?
   |
help: possible candidate is found in another module, you can import it into scope
   |
7  | use style::gecko_bindings::bindings::Servo_ImportRule_Debug;
   |

error[E0425]: cannot find value `Servo_Keyframe_Debug` in this scope
  --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:93:7
   |
93 |     [ Servo_Keyframe_Debug, bindings::Servo_Keyframe_Debug ];
   |       ^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_Keyframe_AddRef`?
   |
help: possible candidate is found in another module, you can import it into scope
   |
7  | use style::gecko_bindings::bindings::Servo_Keyframe_Debug;
   |

error[E0425]: cannot find value `Servo_KeyframesRule_Debug` in this scope
  --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:96:7
   |
96 |     [ Servo_KeyframesRule_Debug, bindings::Servo_KeyframesRule_Debug ];
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_KeyframesRule_AddRef`?
   |
help: possible candidate is found in another module, you can import it into scope
   |
7  | use style::gecko_bindings::bindings::Servo_KeyframesRule_Debug;
   |

error[E0425]: cannot find value `Servo_MediaRule_Debug` in this scope
  --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:99:7
   |
99 |     [ Servo_MediaRule_Debug, bindings::Servo_MediaRule_Debug ];
   |       ^^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_MediaRule_AddRef`?
   |
help: possible candidate is found in another module, you can import it into scope
   |
7  | use style::gecko_bindings::bindings::Servo_MediaRule_Debug;
   |

error[E0425]: cannot find value `Servo_NamespaceRule_Debug` in this scope
   --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:103:7
    |
103 |     [ Servo_NamespaceRule_Debug, bindings::Servo_NamespaceRule_Debug ];
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_NamespaceRule_GetURI`?
    |
help: possible candidate is found in another module, you can import it into scope
    |
7   | use style::gecko_bindings::bindings::Servo_NamespaceRule_Debug;
    |

error[E0425]: cannot find value `Servo_PageRule_Debug` in this scope
   --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:106:7
    |
106 |     [ Servo_PageRule_Debug, bindings::Servo_PageRule_Debug ];
    |       ^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_PageRule_AddRef`?
    |
help: possible candidate is found in another module, you can import it into scope
    |
7   | use style::gecko_bindings::bindings::Servo_PageRule_Debug;
    |

error[E0425]: cannot find value `Servo_SupportsRule_Debug` in this scope
   --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:109:7
    |
109 |     [ Servo_SupportsRule_Debug, bindings::Servo_SupportsRule_Debug ];
    |       ^^^^^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_SupportsRule_AddRef`?
    |
help: possible candidate is found in another module, you can import it into scope
    |
7   | use style::gecko_bindings::bindings::Servo_SupportsRule_Debug;
    |

error[E0425]: cannot find value `Servo_DocumentRule_Debug` in this scope
   --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:113:7
    |
113 |     [ Servo_DocumentRule_Debug, bindings::Servo_DocumentRule_Debug ];
    |       ^^^^^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_DocumentRule_AddRef`?
    |
help: possible candidate is found in another module, you can import it into scope
    |
7   | use style::gecko_bindings::bindings::Servo_DocumentRule_Debug;
    |

error[E0425]: cannot find value `Servo_FontFeatureValuesRule_Debug` in this scope
   --> /home/servo/buildbot/slave/linux-rel-css/build/target/geckolib/release/build/stylo_tests-161c816f485961cf/out/check_bindings.rs:117:7
    |
117 |     [ Servo_FontFeatureValuesRule_Debug, bindings::Servo_FontFeatureValuesRule_Debug ];
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ did you mean `Servo_FontFeatureValuesRule_AddRef`?
    |
help: possible candidate is found in another module, you can import it into scope
    |
7   | use style::gecko_bindings::bindings::Servo_FontFeatureValuesRule_Debug;

In total, this PR saves ~60k. The conditional compilation on the _Debug FFI function eliminates one of the ToCss variants, eliminating 54.4k, as well as a bunch of other <1k functions. Removing the public trait implementation of Debug for the font metrics provider eliminates the last Debug impl from stylo. MozReview-Commit-ID: nIfQ3sy4OW

bholley · 2018-01-16T01:24:56Z

@bors-servo r=emilio

bors-servo · 2018-01-16T01:24:56Z

📌 Commit 9a8821a has been approved by emilio

bors-servo · 2018-01-16T01:25:01Z

⌛ Testing commit 9a8821a with merge 7de87c4...

Avoid debug serialization of PropertyDeclaration & co in release builds In total, this PR saves ~60k. The conditional compilation on the _Debug FFI function eliminates one of the ToCss variants, eliminating 54.4k, as well as a bunch of other <1k functions. Removing the public trait implementation of Debug for the font metrics provider eliminates the last Debug impl from stylo.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19779)

bors-servo · 2018-01-16T02:14:17Z

☀️ 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 7de87c4 to master...

highfive assigned glennw Jan 15, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 15, 2018

bholley force-pushed the avoid_rule_debug_hooks branch from f692ea6 to 903cf60 Compare January 15, 2018 23:48

emilio approved these changes Jan 15, 2018

View reviewed changes

highfive assigned emilio and unassigned glennw Jan 15, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 15, 2018

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 16, 2018

bholley force-pushed the avoid_rule_debug_hooks branch from 903cf60 to 9a8821a Compare January 16, 2018 01:24

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 16, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 16, 2018

bors-servo merged commit 9a8821a into servo:master Jan 16, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 16, 2018

SimonSapin mentioned this pull request Feb 6, 2018

Use specific debug assertion #19962

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid debug serialization of PropertyDeclaration & co in release builds #19779

Avoid debug serialization of PropertyDeclaration & co in release builds #19779

bholley commented Jan 15, 2018 •

edited by SimonSapin

highfive commented Jan 15, 2018

highfive commented Jan 15, 2018

emilio left a comment

emilio commented Jan 15, 2018

bors-servo commented Jan 15, 2018

bors-servo commented Jan 15, 2018

bors-servo commented Jan 16, 2018

jdm commented Jan 16, 2018

bholley commented Jan 16, 2018

bors-servo commented Jan 16, 2018

bors-servo commented Jan 16, 2018

bors-servo commented Jan 16, 2018

Avoid debug serialization of PropertyDeclaration & co in release builds #19779

Avoid debug serialization of PropertyDeclaration & co in release builds #19779

Conversation

bholley commented Jan 15, 2018 • edited by SimonSapin

highfive commented Jan 15, 2018

highfive commented Jan 15, 2018

emilio left a comment

Choose a reason for hiding this comment

emilio commented Jan 15, 2018

bors-servo commented Jan 15, 2018

bors-servo commented Jan 15, 2018

bors-servo commented Jan 16, 2018

jdm commented Jan 16, 2018

bholley commented Jan 16, 2018

bors-servo commented Jan 16, 2018

bors-servo commented Jan 16, 2018

bors-servo commented Jan 16, 2018

bholley commented Jan 15, 2018 •

edited by SimonSapin