Add lots of Arc’s in style, and prepare for using DOMRefCell #13134

SimonSapin · 2016-08-31T00:36:19Z

DOMRefCell usage is not there year because of thread-safety questions, but I have this much already that I’d like to land before it bitrots.

r? @emilio

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require new tests because refactor

This change is


        Remove CSSRule::Charset

`@charset` rules are not reflected in CSSOM.


        Add a separate NamespaceRule type


        Wrap in Arc<_> every object reflected in CSSOM.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1281962#c5


        Update our copy of RefCell, add try_borrow and try_borrow_mut.


        Make DOMRefCell use style’s copy of RefCell


        Move DOMRefCell to style.


        Import Debug implementations for RefCell and friends


        Use Arc<PropertyDeclarationBlock> everwhere it’s appropriate.


        Remove now-redundant Arc inside PropertyDeclarationBlock.


        Prepare for interior mutability in PropertyDeclarationBlock

`Stylist` contains separate hashmaps for important and normal declarations, but typically a given block only contains declarations of one importance. Before this commit, there is an optimization where a `PropertyDeclarationBlock` is only inserted in the corresponding map if it has a non-zero number of declaration of a given importance. With CSSOM, `PropertyDeclarationBlock` is gonna have interior mutability: the importance (priority) of a declaration could change. This optimization would become incorrect when the block is missing in a hashmap where it should be. This commits removes the original optimization, and replaces it with a slightly weaker one: if a block doesn’t have any declaration with the importance we’re cascading for, skip selector matching.

SimonSapin · 2016-08-31T00:39:04Z

@bors-servo try

bors-servo · 2016-08-31T00:39:07Z

⌛ Trying commit 7ef4930 with merge 0f4b7b0...


        Auto merge of #13134 - servo:archery, r=<try>

@emilio

Add lots of Arc’s in style, and prepare for using DOMRefCell  `DOMRefCell` usage is not there year because of thread-safety questions, but I have this much already that I’d like to land before it bitrots. r? @emilio ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require new tests because refactor   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13134)

emilio · 2016-08-31T00:49:32Z

components/script/dom/element.rs

@@ -330,7 +330,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
        fn from_declaration(rule: PropertyDeclaration) -> DeclarationBlock {
            DeclarationBlock::from_declarations(
                Arc::new(PropertyDeclarationBlock {
-                    declarations: Arc::new(vec![(rule, Importance::Normal)]),


heh, was about to ask for this while reviewing the previous commit.

emilio · 2016-08-31T00:51:37Z

ports/geckolib/wrapper.rs

-                unsafe { transmute(&declarations.declarations) }
-            })
+            let opt_ptr = GeckoDeclarationBlock::with(declarations, |declarations| {
+                // Use a raw poointer to extend the lifetime


Can you comment here why it's fine? (Basically, because we keep a strong reference to it during all the styling process?)

@upsuper, may you know about a better way of describing this? If so, I'd appreciate leaving a reply here so Simon can include it, or adding it in another patch.

Well, I don’t know if this is fine! That’s why I was asking about it on IRC. I don’t really understand the lifetime (in the sense of when it is destroyed) of various values here and in HasArcFFI, so I’m assuming that someone did review this code when it landed.

The raw pointer deref is just less wildly dangerous than the unqualified transmute.

emilio · 2016-08-31T00:55:26Z

components/style/selector_matching.rs

+            let any_declaration_for_importance = if importance.important() {
+                block.important_count > 0
+            } else {
+                block.declarations.len() > block.important_count as usize


I think block.declarations.len() != block.important_count would be clearer (or maybe adding a helper method to get the normal count?)

Moved into a pair of methods.

emilio · 2016-08-31T00:57:14Z

components/style/selector_matching.rs

@@ -165,39 +165,35 @@ impl Stylist {
        // Take apart the StyleRule into individual Rules and insert
        // them into the SelectorMap of that priority.
        macro_rules! append(
-            ($style_rule: ident, $priority: ident, $importance: expr, $count: expr) => {


Could you add a comment here about why we can't rip this out if we know there are no declarations?

Now that I think about it, with this change the two SelectorMaps in a given PerOriginSelectorMap become identical… so we don’t need to have two anymore.

I’ll do that in a new commit and add a comment here that we insert empty blocks in case CSSOM adds something to them later.

emilio · 2016-08-31T01:09:49Z

Looks basically great to me. Please add a few comments to avoid people introducing subtle bugs like on the append! macro, and I think you can pretty much r=me.

Since I think you're not landing this right now, I'd still like to give another careful look, but this is awesome :-).

I'd like @nox or @Ms2ger to sign-off the movement of DOMRefCell (or you to confirm that they're fine with it).

emilio · 2016-08-31T01:12:25Z

components/style/selector_matching.rs

@@ -373,7 +369,8 @@ impl Stylist {
        map.user_agent.normal.get_all_matching_rules(element,
                                                     parent_bf,
                                                     applicable_declarations,
-                                                     &mut relations);


Maybe, since each map contains rules for a given importance, the importance could be stored on the map itself?

It could, but that’s changing (see previous inline comment).

emilio · 2016-08-31T01:17:03Z

@upsuper had a few concerns in terms of how it interacts with the Gecko side, those will need to be discussed before merging, I guess.

upsuper · 2016-08-31T01:58:38Z

ports/geckolib/glue.rs

@@ -356,7 +356,7 @@ pub extern "C" fn Servo_StyleSet_Drop(data: *mut RawServoStyleSet) -> () {
 }

 pub struct GeckoDeclarationBlock {
-    pub declarations: Option<PropertyDeclarationBlock>,
+    pub declarations: Option<Arc<PropertyDeclarationBlock>>,


I'm concerned about this one. GeckoDeclarationBlock is refcounted in the Gecko side, and I thought the whole point of removing Arc inside PropertyDeclarationBlock is to remove the double indirection that a refcounted object holding another refcounted object while they are expected to have the same lifetime.

Do you have plan to remove this Arc as well, or probably we'd better redisign GeckoDeclarationBlock so that we can refcount PropertyDeclarationBlock from the Gecko side directly? Or do we have to live with the double indirection if both changes are hard?

Arc<_> is needed somewhere to allow things like Stylist, SelectorMap, and ApplicableDeclarations to reference blocks without copying them.

Before this PR this was Arc<Vec<(PropertyDeclaration, Importance)>> (one of which was a field of PropertyDeclarationBlock). This PR makes it Arc<PropertyDeclarationBlock>, but the plan is to have it pretty soon be Arc<DOMRefCell<PropertyDeclarationBlock>>.

I’m not sure what you mean by refcount from the Gecko side directly, but to deal with Arc<_> from Gecko you need FFI back into Rust to clone or drop it (increment or decrement the refcount). (Unless you want to make assumptions about the memory representation of private implementation details of Arc, but I strongly recommend against that.)

Yeah, I mean refcounting via FFI. But it isn't quite realistic either, given there are some additional fields in this struct.

I wonder whether it is feasible to make all those things only hold a immutable reference to PropertyDeclarationBlock, so that we don't need to wrap it with Arc? If not, probably we need to live with the double indirection, then.

I don't think so, I think the double indirection is fine for now. If for some reason it becomes a perf issue we can try to be a bit smarter, maybe conditionally compiling the extra Gecko fields inside PropertyDeclarationBlock, or similar.

Edit: Well, that suggestion doesn't make that much sense, but I think doing the simple thing first, then profiling and refactoring if necessary is better than discussing it without any actual numbers.

bors-servo · 2016-08-31T02:13:15Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev


        Add any_important and any_normal methods to PropertyDeclarationBlock

emilio · 2016-08-31T21:30:52Z

@bors-servo: r+

bors-servo · 2016-08-31T21:30:53Z

📌 Commit 95033e1 has been approved by emilio

bors-servo · 2016-08-31T21:30:56Z

⌛ Testing commit 95033e1 with merge bbfe38e...


        Auto merge of #13134 - servo:archery, r=emilio

@emilio

Add lots of Arc’s in style, and prepare for using DOMRefCell  `DOMRefCell` usage is not there year because of thread-safety questions, but I have this much already that I’d like to land before it bitrots. r? @emilio ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require new tests because refactor   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13134)

bors-servo · 2016-08-31T22:17:39Z

💔 Test failed - linux-rel

highfive · 2016-08-31T22:17:42Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

emilio · 2016-08-31T22:27:55Z

@bors-servo retry p=1

#9087, and blocks other PRs

bors-servo · 2016-08-31T22:27:57Z

⚡ Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev are reusable. Rebuilding only linux-rel...

bors-servo · 2016-08-31T23:14:03Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev


        Remove one level of nesting in `Stylist`

Since #13134, the "normal" and "important" parts of `Stylist` are identical, so we don’t need to store them twice.


        Auto merge of #13179 - servo:flatter-stylist, r=emilio

@emilio

Remove one level of nesting in `Stylist`  Since #13134, the "normal" and "important" parts of `Stylist` are identical, so we don’t need to store them twice. r? @emilio ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [x] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13179)

SimonSapin added 10 commits Aug 23, 2016

Remove CSSRule::Charset

a5a3a74

`@charset` rules are not reflected in CSSOM.

Add a separate NamespaceRule type

94d5b28

Wrap in Arc<_> every object reflected in CSSOM.

ef5977f

See https://bugzilla.mozilla.org/show_bug.cgi?id=1281962#c5

Update our copy of RefCell, add try_borrow and try_borrow_mut.

edbd88c

Make DOMRefCell use style’s copy of RefCell

ec72305

Move DOMRefCell to style.

c87180a

Import Debug implementations for RefCell and friends

c50e6ad

Use Arc<PropertyDeclarationBlock> everwhere it’s appropriate.

acc38aa

Remove now-redundant Arc inside PropertyDeclarationBlock.

ad3437b

emilio reviewed Aug 31, 2016
View changes

upsuper reviewed Aug 31, 2016
View changes

Add any_important and any_normal methods to PropertyDeclarationBlock

Loading status checks…

95033e1

highfive added the S-awaiting-review label Aug 31, 2016

highfive assigned emilio Aug 31, 2016

highfive removed the S-awaiting-review label Aug 31, 2016

highfive added the S-awaiting-merge label Aug 31, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 31, 2016

This was referenced Aug 31, 2016

Fix incremental restyling under some circumstances due to our bogus use of ElementFlags. #13110

Merged

Add sugar for handling borrowed and owned types #12991

Merged

Make the dom module private. #11672

Merged

SimonSapin deleted the archery branch Aug 31, 2016

SimonSapin added a commit that referenced this pull request Sep 6, 2016

Remove one level of nesting in `Stylist`

Loading status checks…

74eaf2a

Since #13134, the "normal" and "important" parts of `Stylist` are identical, so we don’t need to store them twice.

SimonSapin mentioned this pull request Sep 6, 2016

Remove one level of nesting in `Stylist` #13179

Merged

3 of 5 tasks complete

servo / servo

Add lots of Arc’s in style, and prepare for using DOMRefCell #13134

Add lots of Arc’s in style, and prepare for using DOMRefCell #13134

SimonSapin commented Aug 31, 2016 •

edited by larsbergstrom

SimonSapin commented Aug 31, 2016

bors-servo commented Aug 31, 2016

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.

emilio commented Aug 31, 2016

This comment has been minimized.

This comment has been minimized.

emilio commented Aug 31, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Aug 31, 2016

emilio commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

highfive commented Aug 31, 2016

emilio commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

servo / servo

Join GitHub today

Add lots of Arc’s in style, and prepare for using DOMRefCell #13134

Add lots of Arc’s in style, and prepare for using DOMRefCell #13134

Conversation

SimonSapin commented Aug 31, 2016 • edited by larsbergstrom

SimonSapin commented Aug 31, 2016

bors-servo commented Aug 31, 2016

This comment has been minimized.

emilio Aug 31, 2016 • edited

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 Aug 31, 2016

This comment has been minimized.

This comment has been minimized.

emilio commented Aug 31, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio Aug 31, 2016 • edited

bors-servo commented Aug 31, 2016

emilio commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

highfive commented Aug 31, 2016

emilio commented Aug 31, 2016

bors-servo commented Aug 31, 2016

bors-servo commented Aug 31, 2016

SimonSapin commented Aug 31, 2016 •

edited by larsbergstrom

emilio Aug 31, 2016 •

edited

emilio Aug 31, 2016 •

edited