Avoid returning / passing around a huge ParsedDeclaration type #16954

SimonSapin · 2017-05-19T16:09:36Z

This enum type used to contain the result of parsing one CSS source declaration (name: value;) and expanding shorthands. Enum types are as big as the biggest of their variant (plus discriminant), which was quite big because some shorthands expand to many longhand properties. This type was returned through many functions and methods, wrapped and rewrapped in Result with different error types. This presumably caused significant memmove traffic.

Instead, we now allocate an ArrayVec on the stack and pass &mut references to it for various functions to push into it. This type is also very big, but we never move it.

We still use an intermediate data structure because we sometimes decide after shorthand expansion that a declaration is invalid after all and that we’re gonna drop it. Only later do we push to a PropertyDeclarationBlock, with an entire ArrayVec or nothing.

In future work we can try to avoid a large stack-allocated array, and instead writing directly to the heap allocation of the Vec inside PropertyDeclarationBlock. However this is tricky: we need to preserve this "all or nothing" aspect of parsing one source declaration, and at the same time we want to make it as little error-prone as possible for the various call sites. PropertyDeclarationBlock curently does property deduplication incrementally: as each PropertyDeclaration is pushed, we check if an existing declaration of the same property exists and if so overwrite it. To get rid of the stack allocated array we’d need to somehow deduplicate separately after pushing multiple PropertyDeclaration.

This change is


        Add test name filtering to 'mach test-stylo'


        Look at size_of ParsedDeclaration… Yeah, we’ll need to fix that.


        Box large components of Longhands structs.

SimonSapin · 2017-05-19T16:10:26Z

CC @bholley

@bors-servo try p=10

bors-servo · 2017-05-19T16:10:31Z

⌛ Trying commit 839b315 with merge ab45bf7...


        Auto merge of #16954 - servo:arrayvec, r=<try>

Avoid returning / passing around a huge ParsedDeclaration type This enum type used to contain the result of parsing one CSS source declaration (`name: value;`) and expanding shorthands. Enum types are as big as the biggest of their variant (plus discriminant), which was quite big because some shorthands expand to many longhand properties. This type was returned through many functions and methods, wrapped and rewrapped in `Result` with different error types. This presumably caused significant `memmove` traffic. Instead, we now allocate an `ArrayVec` on the stack and pass `&mut` references to it for various functions to push into it. This type is also very big, but we never move it. We still use an intermediate data structure because we sometimes decide after shorthand expansion that a declaration is invalid after all and that we’re gonna drop it. Only later do we push to a `PropertyDeclarationBlock`, with an entire `ArrayVec` or nothing. In future work we can try to avoid a large stack-allocated array, and instead writing directly to the heap allocation of the `Vec` inside `PropertyDeclarationBlock`. However this is tricky: we need to preserve this "all or nothing" aspect of parsing one source declaration, and at the same time we want to make it as little error-prone as possible for the various call sites. `PropertyDeclarationBlock` curently does property deduplication incrementally: as each `PropertyDeclaration` is pushed, we check if an existing declaration of the same property exists and if so overwrite it. To get rid of the stack allocated array we’d need to somehow deduplicate separately after pushing multiple `PropertyDeclaration`.

bors-servo · 2017-05-19T16:11:02Z

💔 Test failed - windows-msvc-dev

SimonSapin · 2017-05-19T16:18:57Z

@bors-servo try

bors-servo · 2017-05-19T16:19:01Z

⌛ Trying commit 661ddad with merge 6d918ac...


        Auto merge of #16954 - servo:arrayvec, r=<try>

Avoid returning / passing around a huge ParsedDeclaration type This enum type used to contain the result of parsing one CSS source declaration (`name: value;`) and expanding shorthands. Enum types are as big as the biggest of their variant (plus discriminant), which was quite big because some shorthands expand to many longhand properties. This type was returned through many functions and methods, wrapped and rewrapped in `Result` with different error types. This presumably caused significant `memmove` traffic. Instead, we now allocate an `ArrayVec` on the stack and pass `&mut` references to it for various functions to push into it. This type is also very big, but we never move it. We still use an intermediate data structure because we sometimes decide after shorthand expansion that a declaration is invalid after all and that we’re gonna drop it. Only later do we push to a `PropertyDeclarationBlock`, with an entire `ArrayVec` or nothing. In future work we can try to avoid a large stack-allocated array, and instead writing directly to the heap allocation of the `Vec` inside `PropertyDeclarationBlock`. However this is tricky: we need to preserve this "all or nothing" aspect of parsing one source declaration, and at the same time we want to make it as little error-prone as possible for the various call sites. `PropertyDeclarationBlock` curently does property deduplication incrementally: as each `PropertyDeclaration` is pushed, we check if an existing declaration of the same property exists and if so overwrite it. To get rid of the stack allocated array we’d need to somehow deduplicate separately after pushing multiple `PropertyDeclaration`.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16954)

bors-servo · 2017-05-19T16:43:23Z

💔 Test failed - windows-msvc-dev

emilio

Looks great. r=me with nits

emilio · 2017-05-19T16:43:50Z

components/script/dom/cssstyledeclaration.rs

+
+            // Step 6
+            match result {
+                Ok(()) => {},


nit: This can use if let now.

Also, where did step 7 go?

The spec renumbered (removed an earlier step), and I forgot to renumber steps 8-9 here to 7-8. Done.

emilio · 2017-05-19T16:43:50Z

components/style/properties/properties.mako.rs

@@ -1504,6 +1297,180 @@ impl PropertyDeclaration {
            PropertyDeclaration::Custom(..) => false,
        }
    }
+
+    /// The `in_keyframe_block` parameter controls this:


I don't see an in_keyframe_block parameter anymore, though I think it's preexisting, mind to update it?

It had been renamed context. Fixed.

emilio · 2017-05-19T16:43:50Z

components/style/properties/properties.mako.rs

+}
+
+const MAX_SUB_PROPERTIES_PER_SHORTHAND_EXCEPT_ALL: usize =
+    ${max(len(s.sub_properties) for s in data.shorthands if s.name != "all")};


As you noticed on irc, you can use shorthands_except_all here.

emilio · 2017-05-19T16:43:50Z

components/style/properties/properties.mako.rs

+
+    fn push(&mut self, declaration: PropertyDeclaration) {
+        let over_capacity = self.declarations.push(declaration).is_some();
+        assert!(!over_capacity);


debug_assert!?

emilio · 2017-05-19T16:43:50Z

components/style/properties/properties.mako.rs

+
+/// A stack-allocated vector of `PropertyDeclaration`
+/// large enough to parse one CSS `key: value` declaration.
+/// (Shorthands expand to multiple `PropertyDeclaration`s.)


It'd be nice to elaborate on how it's used here, for example, saying that it's used as a scratch buffer for each key: value in a CSS declaration block.

As discussed on IRC I’m not sure what to add since this already mentions parsing one key: value.


        Avoid returning / passing around a huge ParsedDeclaration type

This enum type used to contain the result of parsing one CSS source declaration (`name: value;`) and expanding shorthands. Enum types are as big as the biggest of their variant (plus discriminant), which was quite big because some shorthands expand to many longhand properties. This type was returned through many functions and methods, wrapped and rewrapped in `Result` with different error types. This presumably caused significant `memmove` traffic. Instead, we now allocate an `ArrayVec` on the stack and pass `&mut` references to it for various functions to push into it. This type is also very big, but we never move it. We still use an intermediate data structure because we sometimes decide after shorthand expansion that a declaration is invalid after all and that we’re gonna drop it. Only later do we push to a `PropertyDeclarationBlock`, with an entire `ArrayVec` or nothing. In future work we can try to avoid a large stack-allocated array, and instead writing directly to the heap allocation of the `Vec` inside `PropertyDeclarationBlock`. However this is tricky: we need to preserve this "all or nothing" aspect of parsing one source declaration, and at the same time we want to make it as little error-prone as possible for the various call sites. `PropertyDeclarationBlock` curently does property deduplication incrementally: as each `PropertyDeclaration` is pushed, we check if an existing declaration of the same property exists and if so overwrite it. To get rid of the stack allocated array we’d need to somehow deduplicate separately after pushing multiple `PropertyDeclaration`.


        Use Vec::reserve in PropertyDeclarationBlock


        Work around CI bustage

SimonSapin · 2017-05-19T16:54:34Z

@bors-servo r=emilio

bors-servo · 2017-05-19T16:54:34Z

📌 Commit 341fc54 has been approved by emilio

bors-servo · 2017-05-19T20:06:34Z

⌛ Testing commit 341fc54 with merge 6f7b1d8...


        Auto merge of #16954 - servo:arrayvec, r=emilio

Avoid returning / passing around a huge ParsedDeclaration type This enum type used to contain the result of parsing one CSS source declaration (`name: value;`) and expanding shorthands. Enum types are as big as the biggest of their variant (plus discriminant), which was quite big because some shorthands expand to many longhand properties. This type was returned through many functions and methods, wrapped and rewrapped in `Result` with different error types. This presumably caused significant `memmove` traffic. Instead, we now allocate an `ArrayVec` on the stack and pass `&mut` references to it for various functions to push into it. This type is also very big, but we never move it. We still use an intermediate data structure because we sometimes decide after shorthand expansion that a declaration is invalid after all and that we’re gonna drop it. Only later do we push to a `PropertyDeclarationBlock`, with an entire `ArrayVec` or nothing. In future work we can try to avoid a large stack-allocated array, and instead writing directly to the heap allocation of the `Vec` inside `PropertyDeclarationBlock`. However this is tricky: we need to preserve this "all or nothing" aspect of parsing one source declaration, and at the same time we want to make it as little error-prone as possible for the various call sites. `PropertyDeclarationBlock` curently does property deduplication incrementally: as each `PropertyDeclaration` is pushed, we check if an existing declaration of the same property exists and if so overwrite it. To get rid of the stack allocated array we’d need to somehow deduplicate separately after pushing multiple `PropertyDeclaration`.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16954)

bors-servo · 2017-05-19T20:21:48Z

💔 Test failed - mac-dev-unit

SimonSapin · 2017-05-19T20:41:48Z

@bors-servo retry

bors-servo · 2017-05-19T23:37:16Z

⌛ Testing commit 341fc54 with merge 60682cf...


        Auto merge of #16954 - servo:arrayvec, r=emilio

Avoid returning / passing around a huge ParsedDeclaration type This enum type used to contain the result of parsing one CSS source declaration (`name: value;`) and expanding shorthands. Enum types are as big as the biggest of their variant (plus discriminant), which was quite big because some shorthands expand to many longhand properties. This type was returned through many functions and methods, wrapped and rewrapped in `Result` with different error types. This presumably caused significant `memmove` traffic. Instead, we now allocate an `ArrayVec` on the stack and pass `&mut` references to it for various functions to push into it. This type is also very big, but we never move it. We still use an intermediate data structure because we sometimes decide after shorthand expansion that a declaration is invalid after all and that we’re gonna drop it. Only later do we push to a `PropertyDeclarationBlock`, with an entire `ArrayVec` or nothing. In future work we can try to avoid a large stack-allocated array, and instead writing directly to the heap allocation of the `Vec` inside `PropertyDeclarationBlock`. However this is tricky: we need to preserve this "all or nothing" aspect of parsing one source declaration, and at the same time we want to make it as little error-prone as possible for the various call sites. `PropertyDeclarationBlock` curently does property deduplication incrementally: as each `PropertyDeclaration` is pushed, we check if an existing declaration of the same property exists and if so overwrite it. To get rid of the stack allocated array we’d need to somehow deduplicate separately after pushing multiple `PropertyDeclaration`.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16954)

bors-servo · 2017-05-20T01:09:47Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: emilio
Pushing 60682cf to master...

SimonSapin added 3 commits May 19, 2017

Add test name filtering to 'mach test-stylo'

389073b

Look at size_of ParsedDeclaration… Yeah, we’ll need to fix that.

5c57ff8

Box large components of Longhands structs.

e4f2413

highfive added S-tests-failed S-awaiting-review and removed S-tests-failed labels May 19, 2017

highfive added the S-tests-failed label May 19, 2017

emilio approved these changes May 19, 2017

View changes

SimonSapin added 3 commits May 19, 2017

Use Vec::reserve in PropertyDeclarationBlock

cb8f778

Work around CI bustage

Loading status checks…

341fc54

SimonSapin force-pushed the arrayvec branch from 661ddad to 341fc54 May 19, 2017

highfive removed the S-tests-failed label May 19, 2017

highfive assigned emilio May 19, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels May 19, 2017

highfive removed the S-awaiting-merge label May 19, 2017

highfive added the S-tests-failed label May 19, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels May 19, 2017

highfive removed the S-awaiting-merge label May 20, 2017

bors-servo mentioned this pull request May 20, 2017

Update app_units to 0.4.1 #16914

Merged

emilio deleted the arrayvec branch May 20, 2017

servo / servo

Avoid returning / passing around a huge ParsedDeclaration type #16954

Avoid returning / passing around a huge ParsedDeclaration type #16954

SimonSapin commented May 19, 2017 •

edited by larsbergstrom

SimonSapin commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

SimonSapin commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

SimonSapin commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 20, 2017

servo / servo

Join GitHub today

Avoid returning / passing around a huge ParsedDeclaration type #16954

Avoid returning / passing around a huge ParsedDeclaration type #16954

Conversation

SimonSapin commented May 19, 2017 • edited by larsbergstrom

SimonSapin commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

SimonSapin commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

SimonSapin commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 20, 2017

SimonSapin commented May 19, 2017 •

edited by larsbergstrom