Use parking_lot::RwLock for PropertyDeclarationBlock #13459

SimonSapin · 2016-09-27T13:59:58Z

As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1305141

Closes #13176

Original PR title: Stop relying on impl<T: HeapSizeOf> HeapSizeOf for Arc<T>
servo/heapsize#37 (comment)

This builds on top of that.

./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 tests because refactor

This change is

highfive · 2016-09-27T14:00:02Z

Heads up! This PR modifies the following files:

@bholley: components/style/Cargo.toml, components/style/restyle_hints.rs, components/style/properties/properties.mako.rs, components/style/selector_matching.rs, components/style/stylesheets.rs, components/style/keyframes.rs
@KiChjang: components/script/dom/document.rs, components/script/dom/htmllinkelement.rs, components/script/dom/element.rs, components/script/dom/htmlstyleelement.rs, components/script/Cargo.toml, components/script/dom/workerglobalscope.rs, components/script/dom/htmlmetaelement.rs, components/script/dom/htmlimageelement.rs, components/script_layout_interface/Cargo.toml, components/script/dom/htmlobjectelement.rs

nox · 2016-09-27T14:01:37Z

@bors-servo r+

bors-servo · 2016-09-27T14:01:38Z

📌 Commit a710b40 has been approved by nox

nox · 2016-09-28T09:48:14Z

Reviewed 3 of 3 files at r1, 12 of 12 files at r2, 8 of 8 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

emilio

I'm fine with this. In general, I'd like us to ensure we don't block in most cases (it should only be needed for keyframes), but I see why that's hard. It can become a followup. In any case it's better than the YOLO RefCell we have now.

emilio · 2016-09-28T12:29:48Z

components/script/dom/cssstyledeclaration.rs

@@ -119,43 +118,42 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {

        // Step 2
        if let Some(shorthand) = Shorthand::from_name(&property) {
+            let style_attribute = owner.style_attribute().borrow();
+            let style_attribute = if let Some(ref style_attribute) = *style_attribute {


This might be easier to read with a match instead?

SimonSapin · 2016-09-28T12:58:23Z

@bors-servo r=emilio p=10

(Prioritized as this is prone to bitrot.)

bors-servo · 2016-09-28T12:58:24Z

📌 Commit d6538f9 has been approved by emilio

bors-servo · 2016-09-28T12:59:18Z

⌛ Testing commit d6538f9 with merge 0e870bf...

Use parking_lot::RwLock for PropertyDeclarationBlock  As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1305141 Closes #13176 --- Original PR title: Stop relying on `impl<T: HeapSizeOf> HeapSizeOf for Arc<T>` servo/heapsize#37 (comment) This builds on top of that. ---  - [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 refactor   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13459)

bors-servo · 2016-09-28T13:09:32Z

💔 Test failed - linux-dev

SimonSapin · 2016-10-04T13:13:58Z

Rebased and added a "Disable applicable declarations cache for elements with a style attribute" commit.

We’ll need to do something (maybe mark the cache as dirty whenever a PropertyDeclarationBlock is mutated?) when more of CSSOM is implemented, but in the meantime I’d like to land this before it bit-rots again.

re-r? @emilio

emilio · 2016-10-04T14:03:32Z

components/style/matching.rs

@@ -502,7 +502,8 @@ trait PrivateMatchMethods: TNode {
                                            -> Arc<ComputedValues>
        where Ctx: StyleContext<'a>
    {
-        let mut cacheable = true;
+        let has_style_attribute = self.as_element().map_or(false, |e| e.style_attribute().is_some());


Can you add a comment here about why we need to do that? Something like the commit message might work. I think the consequences of removing are non-trivial enough that it deserves it.

This commit is r=me with that.

emilio · 2016-10-04T14:04:27Z

Also, I think this is the only not-previously-reviewed commit, so feel free to r=me the whole PR when that comment is added.

@bors-servo: try

bors-servo · 2016-10-04T14:04:29Z

⌛ Trying commit 3d1a917 with merge fc84d1e...

Use parking_lot::RwLock for PropertyDeclarationBlock  As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1305141 Closes #13176 --- Original PR title: Stop relying on `impl<T: HeapSizeOf> HeapSizeOf for Arc<T>` servo/heapsize#37 (comment) This builds on top of that. ---  - [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 refactor   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13459)

bors-servo · 2016-10-04T15:29:05Z

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

…bute. The cache is never effective in this case since a style attribute adds to the set of applicable declarations. This also avoid avoids the cache giving out-dated (incorrect) results when `PropertyDeclarationBlock` becomes mutable in a later commit. This is sufficient for now since CSSOM is only implemented for `Element.style`. When StyleRule mutation is added, some additional mechanism (such as evicting the cache) will be needed.

…Block

We’re not using it in style after all.

This reverts commit ec72305.

SimonSapin · 2016-10-04T16:36:10Z

Yes, that was the only new significant change.

@bors-servo r=emilio

bors-servo · 2016-10-04T16:36:11Z

📌 Commit d15ac9b has been approved by emilio

bors-servo · 2016-10-04T16:36:25Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Add support for 'font-stretch' in geckolib #13570

bors-servo · 2016-10-04T16:36:25Z

📌 Commit d15ac9b has been approved by emilio

bors-servo · 2016-10-04T16:58:57Z

⌛ Testing commit d15ac9b with merge aea9545...

Use parking_lot::RwLock for PropertyDeclarationBlock  As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1305141 Closes #13176 --- Original PR title: Stop relying on `impl<T: HeapSizeOf> HeapSizeOf for Arc<T>` servo/heapsize#37 (comment) This builds on top of that. ---  - [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 refactor   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13459)

bors-servo · 2016-10-04T17:52:22Z

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

highfive assigned KiChjang Sep 27, 2016

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

SimonSapin changed the title ~~Make style build without impl<T: HeapSizeOf> HeapSizeOf for Arc<T>.~~ Stop relying on impl<T: HeapSizeOf> HeapSizeOf for Arc<T>. Sep 27, 2016

highfive assigned nox and unassigned KiChjang Sep 27, 2016

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 27, 2016

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 Sep 28, 2016

SimonSapin changed the title ~~Stop relying on impl<T: HeapSizeOf> HeapSizeOf for Arc<T>.~~ Use parking_lot::RwLock for PropertyDeclarationBlock Sep 28, 2016

SimonSapin force-pushed the no-arc-heapsize branch from 961eb33 to 22b2668 Compare September 28, 2016 09:55

emilio approved these changes Sep 28, 2016

View reviewed changes

SimonSapin force-pushed the no-arc-heapsize branch from bb79914 to d6538f9 Compare September 28, 2016 12:57

highfive assigned emilio and unassigned nox Sep 28, 2016

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 28, 2016

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 28, 2016

SimonSapin force-pushed the no-arc-heapsize branch from d6538f9 to 90d245a Compare September 28, 2016 13:15

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

SimonSapin force-pushed the no-arc-heapsize branch from 90d245a to 3d1a917 Compare October 4, 2016 13:11

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 Oct 4, 2016

emilio reviewed Oct 4, 2016

View reviewed changes

SimonSapin added 6 commits October 4, 2016 18:34

Use PropertyDeclarationBlock in a DOMRefCell everywhere.

d986fd2

Use parking_lot::RwLock instead of DOMRefCell for PropertyDeclaration…

89a29a7

…Block

Move DOMRefCell back into script.

c831369

We’re not using it in style after all.

Revert "Make DOMRefCell use style’s copy of RefCell"

fbde392

This reverts commit ec72305.

Don’t pretend DOMRefCell is Send or Sync, after all.

d15ac9b

SimonSapin force-pushed the no-arc-heapsize branch from 3d1a917 to d15ac9b Compare October 4, 2016 16:35

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 Oct 4, 2016

bors-servo merged commit d15ac9b into master Oct 4, 2016

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

SimonSapin deleted the no-arc-heapsize branch October 4, 2016 17:53

This was referenced Oct 4, 2016

Switch to serde_derive #13582

Closed

Make TopTypeId an untagged union #13224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use parking_lot::RwLock for PropertyDeclarationBlock #13459

Use parking_lot::RwLock for PropertyDeclarationBlock #13459

SimonSapin commented Sep 27, 2016 •

edited

Loading

highfive commented Sep 27, 2016

nox commented Sep 27, 2016

bors-servo commented Sep 27, 2016

nox commented Sep 28, 2016

emilio left a comment

emilio Sep 28, 2016

SimonSapin commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

SimonSapin commented Oct 4, 2016

emilio Oct 4, 2016

emilio commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

SimonSapin commented Oct 4, 2016 •

edited

Loading

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

Use parking_lot::RwLock for PropertyDeclarationBlock #13459

Use parking_lot::RwLock for PropertyDeclarationBlock #13459

Conversation

SimonSapin commented Sep 27, 2016 • edited Loading

highfive commented Sep 27, 2016

nox commented Sep 27, 2016

bors-servo commented Sep 27, 2016

nox commented Sep 28, 2016

emilio left a comment

Choose a reason for hiding this comment

emilio Sep 28, 2016

Choose a reason for hiding this comment

SimonSapin commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

SimonSapin commented Oct 4, 2016

emilio Oct 4, 2016

Choose a reason for hiding this comment

emilio commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

SimonSapin commented Oct 4, 2016 • edited Loading

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

SimonSapin commented Sep 27, 2016 •

edited

Loading

SimonSapin commented Oct 4, 2016 •

edited

Loading