(Do not merge yet) Per-document locks for CSSOM objects #15998

SimonSapin · 2017-03-17T00:17:44Z

… or per-process in Stylo.

This is not done, I’m opening a PR to see what happens on try.

This change is

SimonSapin · 2017-03-17T00:17:55Z

bors-servo · 2017-03-17T00:17:58Z

⌛ Trying commit 5ffa20c with merge 11b1213...


        Auto merge of #15998 - servo:style-ref, r=<try>

(Do not merge yet) Per-document locks for CSSOM objects … or per-process in Stylo. This is not done, I’m opening a PR to see what happens on try.

emilio · 2017-03-17T00:19:48Z

It'd be nice to get some numbers too, though I guess it's hard for it to be slower than Arc<RwLock<...>>

SimonSapin · 2017-03-17T00:27:36Z

So far I don’t expect the numbers to be significantly different. It’s when PropertyDeclarationBlocks will be moved over and RwLock::read inside restyling loops will be replaced with Locked::read_with that we’ll (hopefully) see benefits. (The former is at least an atomic write, the latter a pointer comparison with a guard obtained outside the loop.)

emilio · 2017-03-17T00:28:27Z

components/style/shared_lock.rs

+                "StyleRef::write_with called with a guard from an unrelated SharedRwLock");
+        let ptr = self.data.get();
+
+        // Unsafe:


What prevents you from doing:

let mut first = locked.write_with(&mut guard); let mut second = locked.write_with(&mut guard); // ^ aliasing

You can’t take two mutable borrows of the same guard.

In fn write_with<'a>(&'a self, guard: &'a mut SharedRwLockWriteGuard) -> &'a mut T the return value borrows both self and guard (since they use the same lifetime parameter).

bors-servo · 2017-03-17T00:42:07Z

💔 Test failed - linux-dev

bors-servo · 2017-03-17T03:48:29Z

☔ The latest upstream changes (presumably #15993) made this pull request unmergeable. Please resolve the merge conflicts.


        Add SharedRwLock<T> and StyleRef<T>


        Replace RwLock<MediaList> with shared_lock::Locked<MediaList>


        Move shared lock acquires up the call stack. (Or is it down?)


        More lock acquire in callers


        Introduce a ToCssWithGuard trait


        Replace more RwLock<MediaList> with shared_lock::Locked<MediaList>


        Tidy

SimonSapin · 2017-03-17T08:33:22Z

@bors-servo try

bors-servo · 2017-03-17T08:33:26Z

⌛ Trying commit 10ea25e with merge 12602f7...


        Auto merge of #15998 - servo:style-ref, r=<try>

(Do not merge yet) Per-document locks for CSSOM objects … or per-process in Stylo. This is not done, I’m opening a PR to see what happens on try.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15998)

SimonSapin · 2017-03-17T08:38:37Z

Trying to unclog the queue, issue unrelated to this PR.

@bors-servo try-

SimonSapin · 2017-03-17T08:39:07Z

@bors-servo r-

SimonSapin · 2017-03-17T09:21:16Z

Turns out you can’t reopen a closed PR after pushing to that branch while the PR was closed…


        Auto merge of #16014 - servo:style-ref, r=<try>

Per-process lock for CSSOM objects  Before this PR, every object reflected in CSSOM is in `Arc<RwLock<_>>` to enable safe (synchronized) mutable aliasing. Acquiring all these locks has significant cost during selector matching: * https://bugzilla.mozilla.org/show_bug.cgi?id=1311469 * https://bugzilla.mozilla.org/show_bug.cgi?id=1335941 * https://bugzilla.mozilla.org/show_bug.cgi?id=1339703 This PR introduce a mechanism to protect many objects with the same `RwLock` that only needs to be acquired once. In Stylo, there is one such lock per process (in a `lazy_static`), used for everything. I non-Stylo Servo, I originally intended to have one such lock per document (for author-origin stylesheets, and one per process for user-agent and user sytlesheets since they’re shared across documents, and never mutated anyway). However I failed to have the same document-specific (or pipeline-specific) `Arc` reachable from both `Document` nodes and `LayoutThread`. Recursively following callers lead me to include this `Arc` in `UnprivilegedPipelineContent`, but that needs to be serializable. So there is a second process-wide lock. This was previously #15998, closed accidentally. ---  - [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 - [ ] These changes do not require tests because _____


        Auto merge of #16014 - servo:style-ref, r=<try>

Per-process lock for CSSOM objects  Before this PR, every object reflected in CSSOM is in `Arc<RwLock<_>>` to enable safe (synchronized) mutable aliasing. Acquiring all these locks has significant cost during selector matching: * https://bugzilla.mozilla.org/show_bug.cgi?id=1311469 * https://bugzilla.mozilla.org/show_bug.cgi?id=1335941 * https://bugzilla.mozilla.org/show_bug.cgi?id=1339703 This PR introduce a mechanism to protect many objects with the same `RwLock` that only needs to be acquired once. In Stylo, there is one such lock per process (in a `lazy_static`), used for everything. I non-Stylo Servo, I originally intended to have one such lock per document (for author-origin stylesheets, and one per process for user-agent and user sytlesheets since they’re shared across documents, and never mutated anyway). However I failed to have the same document-specific (or pipeline-specific) `Arc` reachable from both `Document` nodes and `LayoutThread`. Recursively following callers lead me to include this `Arc` in `UnprivilegedPipelineContent`, but that needs to be serializable. So there is a second process-wide lock. This was previously #15998, closed accidentally. ---  - [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 - [ ] 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/16014)


        Auto merge of #16014 - servo:style-ref, r=<try>

Per-process lock for CSSOM objects  Before this PR, every object reflected in CSSOM is in `Arc<RwLock<_>>` to enable safe (synchronized) mutable aliasing. Acquiring all these locks has significant cost during selector matching: * https://bugzilla.mozilla.org/show_bug.cgi?id=1311469 * https://bugzilla.mozilla.org/show_bug.cgi?id=1335941 * https://bugzilla.mozilla.org/show_bug.cgi?id=1339703 This PR introduce a mechanism to protect many objects with the same `RwLock` that only needs to be acquired once. In Stylo, there is one such lock per process (in a `lazy_static`), used for everything. I non-Stylo Servo, I originally intended to have one such lock per document (for author-origin stylesheets, and one per process for user-agent and user sytlesheets since they’re shared across documents, and never mutated anyway). However I failed to have the same document-specific (or pipeline-specific) `Arc` reachable from both `Document` nodes and `LayoutThread`. Recursively following callers lead me to include this `Arc` in `UnprivilegedPipelineContent`, but that needs to be serializable. So there is a second process-wide lock. This was previously #15998, closed accidentally. ---  - [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 - [ ] 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/16014)


        Auto merge of #16014 - servo:style-ref, r=<try>

Per-process lock for CSSOM objects  Before this PR, every object reflected in CSSOM is in `Arc<RwLock<_>>` to enable safe (synchronized) mutable aliasing. Acquiring all these locks has significant cost during selector matching: * https://bugzilla.mozilla.org/show_bug.cgi?id=1311469 * https://bugzilla.mozilla.org/show_bug.cgi?id=1335941 * https://bugzilla.mozilla.org/show_bug.cgi?id=1339703 This PR introduce a mechanism to protect many objects with the same `RwLock` that only needs to be acquired once. In Stylo, there is one such lock per process (in a `lazy_static`), used for everything. I non-Stylo Servo, I originally intended to have one such lock per document (for author-origin stylesheets, and one per process for user-agent and user sytlesheets since they’re shared across documents, and never mutated anyway). However I failed to have the same document-specific (or pipeline-specific) `Arc` reachable from both `Document` nodes and `LayoutThread`. Recursively following callers lead me to include this `Arc` in `UnprivilegedPipelineContent`, but that needs to be serializable. So there is a second process-wide lock. This was previously #15998, closed accidentally. ---  - [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 - [ ] 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/16014)


        Auto merge of #16014 - servo:style-ref, r=emilio

Per-process lock for CSSOM objects  Before this PR, every object reflected in CSSOM is in `Arc<RwLock<_>>` to enable safe (synchronized) mutable aliasing. Acquiring all these locks has significant cost during selector matching: * https://bugzilla.mozilla.org/show_bug.cgi?id=1311469 * https://bugzilla.mozilla.org/show_bug.cgi?id=1335941 * https://bugzilla.mozilla.org/show_bug.cgi?id=1339703 This PR introduce a mechanism to protect many objects with the same `RwLock` that only needs to be acquired once. In Stylo, there is one such lock per process (in a `lazy_static`), used for everything. I non-Stylo Servo, I originally intended to have one such lock per document (for author-origin stylesheets, and one per process for user-agent and user sytlesheets since they’re shared across documents, and never mutated anyway). However I failed to have the same document-specific (or pipeline-specific) `Arc` reachable from both `Document` nodes and `LayoutThread`. Recursively following callers lead me to include this `Arc` in `UnprivilegedPipelineContent`, but that needs to be serializable. So there is a second process-wide lock. This was previously #15998, closed accidentally. ---  - [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 - [ ] 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/16014)

emilio reviewed Mar 17, 2017

View changes

highfive added the S-tests-failed label Mar 17, 2017

highfive added the S-needs-rebase label Mar 17, 2017

SimonSapin added 7 commits Feb 11, 2017

Add SharedRwLock<T> and StyleRef<T>

d014087

Replace RwLock<MediaList> with shared_lock::Locked<MediaList>

ca0bb5e

Move shared lock acquires up the call stack. (Or is it down?)

d8d1050

More lock acquire in callers

c762b23

Introduce a ToCssWithGuard trait

591c101

Replace more RwLock<MediaList> with shared_lock::Locked<MediaList>

6c9919d

Tidy

Loading status checks…

10ea25e

SimonSapin force-pushed the style-ref branch from 5ffa20c to 10ea25e Mar 17, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Mar 17, 2017

SimonSapin closed this Mar 17, 2017

SimonSapin mentioned this pull request Mar 18, 2017

Per-process lock for CSSOM objects #16014

Merged

2 of 5 tasks complete

emilio mentioned this pull request Mar 18, 2017

selectors: Check the bloom filter at most once per complex selector. #15890

Merged

servo / servo

(Do not merge yet) Per-document locks for CSSOM objects #15998

(Do not merge yet) Per-document locks for CSSOM objects #15998

SimonSapin commented Mar 17, 2017 •

edited by larsbergstrom

SimonSapin commented Mar 17, 2017

bors-servo commented Mar 17, 2017

emilio commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Mar 17, 2017

bors-servo commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

bors-servo commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

servo / servo

Join GitHub today

(Do not merge yet) Per-document locks for CSSOM objects #15998

(Do not merge yet) Per-document locks for CSSOM objects #15998

Conversation

SimonSapin commented Mar 17, 2017 • edited by larsbergstrom

SimonSapin commented Mar 17, 2017

bors-servo commented Mar 17, 2017

emilio commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

This comment has been minimized.

emilio Mar 17, 2017

This comment has been minimized.

SimonSapin Mar 17, 2017

This comment has been minimized.

SimonSapin Mar 17, 2017

bors-servo commented Mar 17, 2017

bors-servo commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

bors-servo commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

SimonSapin commented Mar 17, 2017

SimonSapin commented Mar 17, 2017 •

edited by larsbergstrom