Use SmallBitVec to optimize size of PropertyDeclarationBlock #18431

mbrubeck · 2017-09-09T15:26:17Z

https://bugzilla.mozilla.org/show_bug.cgi?id=1398322

This change is

highfive · 2017-09-09T15:26:22Z

Heads up! This PR modifies the following files:

@bholley: components/style/Cargo.toml, components/style/lib.rs, ports/geckolib/glue.rs, components/style/stylesheets/keyframes_rule.rs, components/style/sharing/mod.rs and 5 more
@canaltinova: components/style/Cargo.toml, components/style/lib.rs, components/style/stylesheets/keyframes_rule.rs, components/style/sharing/mod.rs, components/style/properties/properties.mako.rs and 4 more
@KiChjang: components/script/dom/cssstyledeclaration.rs
@fitzgen: components/script/dom/cssstyledeclaration.rs
@emilio: components/style/Cargo.toml, components/style/lib.rs, ports/geckolib/glue.rs, components/style/stylesheets/keyframes_rule.rs, components/style/sharing/mod.rs and 5 more

highfive · 2017-09-09T15:26:23Z

Warning

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

mbrubeck · 2017-09-09T15:26:45Z

@bors-servo try

bors-servo · 2017-09-09T15:26:50Z

⌛ Trying commit 0f258bb with merge 63ccc03...

WIP: Use SmallBitVec to optimize size of PropertyDeclarationBlock https://bugzilla.mozilla.org/show_bug.cgi?id=1398322  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18431)

bors-servo · 2017-09-09T15:52:19Z

💔 Test failed - windows-msvc-dev

mbrubeck · 2017-09-09T17:13:43Z

@bors-servo try

bors-servo · 2017-09-09T17:13:48Z

⌛ Trying commit 216ebb4 with merge 256152a...

WIP: Use SmallBitVec to optimize size of PropertyDeclarationBlock https://bugzilla.mozilla.org/show_bug.cgi?id=1398322  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18431)

bors-servo · 2017-09-09T18:43:44Z

💔 Test failed - mac-rel-css2

jdm · 2017-09-09T21:18:25Z

  ▶ CRASH [expected FAIL] /css21_dev/html4/first-letter-punctuation-234.htm
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 17.2.0-devel
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ CRASH [expected FAIL] /css21_dev/html4/first-letter-punctuation-236.htm
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 17.2.0-devel
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ CRASH [expected FAIL] /css21_dev/html4/first-letter-punctuation-237.htm

  ▶ CRASH [expected FAIL] /css21_dev/html4/first-letter-punctuation-235.htm
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 17.2.0-devel
  └ Shutting down the Constellation after generating an output file or exit flag specified

mbrubeck · 2017-09-10T04:10:59Z

@bors-servo try

bors-servo · 2017-09-10T04:11:04Z

⌛ Trying commit 606e597 with merge c92de04...

WIP: Use SmallBitVec to optimize size of PropertyDeclarationBlock https://bugzilla.mozilla.org/show_bug.cgi?id=1398322  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18431)

bors-servo · 2017-09-10T04:17:34Z

💔 Test failed - mac-rel-css2

mbrubeck · 2017-09-10T04:22:47Z

@bors-servo retry

infra

bors-servo · 2017-09-10T04:22:52Z

⌛ Trying commit 606e597 with merge e81e46e...

WIP: Use SmallBitVec to optimize size of PropertyDeclarationBlock https://bugzilla.mozilla.org/show_bug.cgi?id=1398322  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18431)

bors-servo · 2017-09-10T06:02:40Z

☀️ 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
State: approved= try=True

mbrubeck · 2017-09-10T17:26:18Z

@bors-servo try-

I think this is ready now. r? @bholley

bholley · 2017-09-10T17:45:43Z

See https://bugzilla.mozilla.org/show_bug.cgi?id=1398322#c14 regarding eq perf, which I think we should probably fix before landing, otherwise revalidation will get slower.

At a high-level this looks great otherwise, but given the unsafe code I think it probably deserves a more-careful review than I have cycles for at the moment, so probably @SimonSapin or @Manishearth should at least review the SmallBitVec code.

mbrubeck · 2017-09-11T13:05:01Z

See https://bugzilla.mozilla.org/show_bug.cgi?id=1398322#c14 regarding eq perf, which I think we should probably fix before landing, otherwise revalidation will get slower.

The latest smallbitvec now has fast paths for eq.

SimonSapin · 2017-09-11T13:30:51Z

r=me on this PR and https://github.com/servo/smallbitvec/blob/v1.0.2/src/lib.rs. While we’re at it, let’s land this with 1.0.3 instead of 1.0.2?

Reviewed 5 of 5 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

components/style/properties/declaration_block.rs, line 81 at r2 (raw file):

    /// The "important" flag for each declaration in `declarations`.
    important_declarations: SmallBitVec,

important_declarations sounds to be like it contains declarations that have the !important flag. Rename this to declarations_importance?

components/style/properties/declaration_block.rs, line 249 at r2 (raw file):

    pub fn get(&self, property: PropertyDeclarationId) -> Option<(&PropertyDeclaration, Importance)> {
        self.declarations.iter().position(|decl| decl.id() == property).map(|i| {
            let decl = &self.declarations[i];

To avoid indexing, could this use .enumerate().find(…) instead of .position(…)?

Comments from Reviewable

mbrubeck · 2017-09-11T13:57:03Z

@bors-servo r=SimonSapin

Addressed all of the review comments above.

bors-servo · 2017-09-11T13:57:04Z

📌 Commit a5a0e9f has been approved by SimonSapin

bors-servo · 2017-09-11T13:57:09Z

⌛ Testing commit a5a0e9f with merge e7f4502...

Use SmallBitVec to optimize size of PropertyDeclarationBlock https://bugzilla.mozilla.org/show_bug.cgi?id=1398322  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18431)

bors-servo · 2017-09-11T14:03:42Z

💔 Test failed - mac-rel-wpt3

mbrubeck · 2017-09-11T14:31:05Z

@bors-servo retry

Infra

bors-servo · 2017-09-11T14:31:09Z

⚡ Previous build results for android, linux-dev, mac-dev-unit, windows-msvc-dev are reusable. Rebuilding only arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4...

bors-servo · 2017-09-11T15:33:20Z

☀️ 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: SimonSapin
Pushing e7f4502 to master...

highfive assigned Manishearth Sep 9, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 9, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 9, 2017

mbrubeck force-pushed the pdb-size branch from 0f258bb to 090e3c1 Compare September 9, 2017 17:04

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 9, 2017

mbrubeck force-pushed the pdb-size branch from 090e3c1 to 216ebb4 Compare September 9, 2017 17:13

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 9, 2017

mbrubeck force-pushed the pdb-size branch from 216ebb4 to 606e597 Compare September 10, 2017 04:10

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 10, 2017

mbrubeck changed the title ~~WIP: Use SmallBitVec to optimize size of PropertyDeclarationBlock~~ Use SmallBitVec to optimize size of PropertyDeclarationBlock Sep 10, 2017

highfive assigned bholley and unassigned Manishearth Sep 10, 2017

mbrubeck added 2 commits September 11, 2017 06:33

Use SmallBitVec to replace BitVec

21006fb

Use SmallBitVec for important flags in PDB

a5a0e9f

mbrubeck force-pushed the pdb-size branch from 606e597 to a5a0e9f Compare September 11, 2017 13:56

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 11, 2017

highfive assigned SimonSapin and unassigned bholley Sep 11, 2017

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 Sep 11, 2017

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 Sep 11, 2017

bors-servo merged commit a5a0e9f into servo:master Sep 11, 2017

bors-servo mentioned this pull request Sep 11, 2017

Make serialization match Gecko in a few corner cases #18352

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SmallBitVec to optimize size of PropertyDeclarationBlock #18431

Use SmallBitVec to optimize size of PropertyDeclarationBlock #18431

mbrubeck commented Sep 9, 2017 •

edited by SimonSapin

Loading

highfive commented Sep 9, 2017

highfive commented Sep 9, 2017

mbrubeck commented Sep 9, 2017

bors-servo commented Sep 9, 2017

bors-servo commented Sep 9, 2017

mbrubeck commented Sep 9, 2017

bors-servo commented Sep 9, 2017

bors-servo commented Sep 9, 2017

jdm commented Sep 9, 2017

mbrubeck commented Sep 10, 2017

bors-servo commented Sep 10, 2017

bors-servo commented Sep 10, 2017

mbrubeck commented Sep 10, 2017

bors-servo commented Sep 10, 2017

bors-servo commented Sep 10, 2017

mbrubeck commented Sep 10, 2017

bholley commented Sep 10, 2017

mbrubeck commented Sep 11, 2017

SimonSapin commented Sep 11, 2017

mbrubeck commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

mbrubeck commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

Use SmallBitVec to optimize size of PropertyDeclarationBlock #18431

Use SmallBitVec to optimize size of PropertyDeclarationBlock #18431

Conversation

mbrubeck commented Sep 9, 2017 • edited by SimonSapin Loading

highfive commented Sep 9, 2017

highfive commented Sep 9, 2017

mbrubeck commented Sep 9, 2017

bors-servo commented Sep 9, 2017

bors-servo commented Sep 9, 2017

mbrubeck commented Sep 9, 2017

bors-servo commented Sep 9, 2017

bors-servo commented Sep 9, 2017

jdm commented Sep 9, 2017

mbrubeck commented Sep 10, 2017

bors-servo commented Sep 10, 2017

bors-servo commented Sep 10, 2017

mbrubeck commented Sep 10, 2017

bors-servo commented Sep 10, 2017

bors-servo commented Sep 10, 2017

mbrubeck commented Sep 10, 2017

bholley commented Sep 10, 2017

mbrubeck commented Sep 11, 2017

SimonSapin commented Sep 11, 2017

mbrubeck commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

mbrubeck commented Sep 11, 2017

bors-servo commented Sep 11, 2017

bors-servo commented Sep 11, 2017

mbrubeck commented Sep 9, 2017 •

edited by SimonSapin

Loading