Set vector properties via iterator in stylo #16721

Manishearth · 2017-05-03T23:43:04Z

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

Avoids transient allocations

This change is

highfive · 2017-05-03T23:43:08Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/properties/data.py, components/style/values/computed/mod.rs, components/style/stylearc.rs, components/style/properties/helpers.mako.rs and 1 more
@emilio: components/style/properties/gecko.mako.rs, components/style/properties/data.py, components/style/values/computed/mod.rs, components/style/stylearc.rs, components/style/properties/helpers.mako.rs and 1 more

highfive · 2017-05-03T23:43:09Z

Warning

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

Manishearth · 2017-05-03T23:43:23Z

r? @emilio

Manishearth · 2017-05-03T23:46:06Z

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

emilio

Generally looks reasonable, but I want to know why the take() business is needed. It adds a fair amount of complexity, which I think should not be needed.

emilio · 2017-05-04T00:41:50Z

components/style/properties/helpers.mako.rs

-                                                                                      specified_value,
-                                                                                      computed,
-                                                                                      inherited_style.get_font());
+                                % if property.is_vector and product == "gecko":


Can we just make the same API for servo and call collect inside the servo function?

emilio · 2017-05-04T00:43:53Z

components/style/properties/helpers.mako.rs

-                                                                                      computed,
-                                                                                      inherited_style.get_font());
+                                % if property.is_vector and product == "gecko":
+                                    let mut s = context.mutate_style().take_${data.current_style_struct.name_lower}();


Why exactly is all this take business needed? I don't see any reason why it should be, right?

(we talked on IRC, and yes, this is needed. can you leave a comment here?)

I guess we rely on the property that a vector property doesn't access its own style struct to compute itself, don't we? Let's have it clearly documented.

(Well, actually I guess they can use self, so we'd be good there too).

In general I don't love the complexity this patch introduces overall, but since I don't have a better suggestion right now, other than the other comments...

emilio · 2017-05-04T00:45:21Z

components/style/stylearc.rs

+///
+/// This lets us build arcs that we can mutate before
+/// freezing, without needing to change the allocation
+pub struct UniqueArc<T: ?Sized>(Arc<T>);


Let's move this out of here, to keep this as close as the original arc.rs as possible.

Meh, actually, I guess it's fine, and being here allows using the private member without unsafety or exposing extra stuff, so...

emilio · 2017-05-04T00:55:35Z

components/style/properties/gecko.mako.rs

-            for (servo, gecko) in v.0.into_iter().zip(self.gecko.mTransitions.iter_mut()) {
+        let v = v.into_iter();
+
+        if v.len() != 0 {


nit: Does ExactSizeIterator support is_empty? If so, let's use it here.

Nope, it's unstable

emilio

Ok, r=me with the nits and the path for servo being made the same (should be a one-liner in the property code).

emilio · 2017-05-04T01:05:27Z

components/style/properties/gecko.mako.rs

+                                     v: I)
+        where I: IntoIterator<Item = longhands::${shorthand}_position_${orientation[0]}::computed_value::single_value::T>,
+              I::IntoIter: ExactSizeIterator
+        {


Indentations is off here.

emilio · 2017-05-04T01:05:34Z

components/style/properties/gecko.mako.rs

+    pub fn set_${shorthand}_image<I>(&mut self, images: I, cacheable: &mut bool)
+        where I: IntoIterator<Item = longhands::${shorthand}_image::computed_value::single_value::T>,
+              I::IntoIter: ExactSizeIterator
+        {


and here (and below)

emilio · 2017-05-04T01:06:44Z

components/style/properties/gecko.mako.rs

+              I::IntoIter: ExactSizeIterator + Clone
+        {
+        let v = v.into_iter();
+        debug_assert!(v.len() != 0);


is_empty. also in all the similar use-cases below

emilio · 2017-05-04T01:08:11Z

components/style/stylearc.rs

+///
+/// This lets us build arcs that we can mutate before
+/// freezing, without needing to change the allocation
+pub struct UniqueArc<T: ?Sized>(Arc<T>);


Meh, actually, I guess it's fine, and being here allows using the private member without unsafety or exposing extra stuff, so...

Manishearth · 2017-05-04T05:38:57Z

@bors-servo r=emilio

bors-servo · 2017-05-04T05:38:57Z

📌 Commit ed0b058 has been approved by emilio

bors-servo · 2017-05-04T05:39:00Z

🔒 Merge conflict

MozReview-Commit-ID: I7oOpYhVP5S

MozReview-Commit-ID: GwOw0HaC9S7

Manishearth · 2017-05-04T15:58:51Z

@bors-servo r=emilio

bors-servo · 2017-05-04T15:58:55Z

📌 Commit 8bc277b has been approved by emilio

bors-servo · 2017-05-04T16:09:57Z

⌛ Testing commit 8bc277b with merge e2b6e9b0ee64839db4fd3425e7c6f0f6cb29d202...

bors-servo · 2017-05-04T16:13:57Z

💔 Test failed - linux-rel-css

Manishearth · 2017-05-04T16:23:01Z

@bors-servo r=emilio

bors-servo · 2017-05-04T16:23:02Z

📌 Commit 33fb27c has been approved by emilio

bors-servo · 2017-05-04T16:23:06Z

⌛ Testing commit 33fb27c with merge e23c8f1...

Set vector properties via iterator in stylo https://bugzilla.mozilla.org/show_bug.cgi?id=1360882 Avoids transient allocations  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16721)

bors-servo · 2017-05-04T17:17:10Z

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

highfive assigned cbrewster May 3, 2017

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

highfive assigned emilio and unassigned cbrewster May 3, 2017

emilio reviewed May 4, 2017

View reviewed changes

Manishearth force-pushed the vector-iter branch 2 times, most recently from 7f3eb81 to ed0b058 Compare May 4, 2017 05:38

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 May 4, 2017

Manishearth added 2 commits May 4, 2017 07:15

Add iterators for vector types

c85aae4

MozReview-Commit-ID: I7oOpYhVP5S

Express unique ownership of arc in StyleStructRef, add vacated state

dd3159f

MozReview-Commit-ID: GwOw0HaC9S7

Manishearth force-pushed the vector-iter branch from ed0b058 to 8bc277b Compare May 4, 2017 15:35

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels May 4, 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 May 4, 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 May 4, 2017

Allow vector properties to be set by iterator

33fb27c

Manishearth force-pushed the vector-iter branch from 8bc277b to 33fb27c Compare May 4, 2017 16:22

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 May 4, 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 May 4, 2017

bors-servo merged commit 33fb27c into servo:master May 4, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 4, 2017

Manishearth deleted the vector-iter branch May 7, 2019 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set vector properties via iterator in stylo #16721

Set vector properties via iterator in stylo #16721

Manishearth commented May 3, 2017 •

edited by larsbergstrom

highfive commented May 3, 2017

highfive commented May 3, 2017

Manishearth commented May 3, 2017

Manishearth commented May 3, 2017

emilio left a comment

emilio May 4, 2017

emilio May 4, 2017

emilio May 4, 2017

emilio May 4, 2017

emilio May 4, 2017

emilio May 4, 2017

emilio May 4, 2017

emilio May 4, 2017

Manishearth May 4, 2017

emilio left a comment

emilio May 4, 2017

emilio May 4, 2017

emilio May 4, 2017

emilio May 4, 2017

Manishearth commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

Manishearth commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

Manishearth commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

Set vector properties via iterator in stylo #16721

Set vector properties via iterator in stylo #16721

Conversation

Manishearth commented May 3, 2017 • edited by larsbergstrom

highfive commented May 3, 2017

highfive commented May 3, 2017

Manishearth commented May 3, 2017

Manishearth commented May 3, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

Manishearth commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

Manishearth commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

bors-servo commented May 4, 2017

Manishearth commented May 3, 2017 •

edited by larsbergstrom