Implement discarding Document objects to reclaim space. #14312

asajeffrey · 2016-11-21T21:29:32Z

This PR implements document discarding. Active documents are kept alive strongly, but inactive documents are only kept alive weakly. When a document is GCd, it is marked as discarded, and if it is every reactivated, a reload of the URL is triggered.

Note that this PR is pretty aggressive about discarding, and can any inactive document (other than those being kept alive by other same-origin pipelines). We might want to damp it down a bit.

Also note that this interacts with browser.html in that the reloading triggered by reactivating a document triggers mozbrowser events.

To test this, I added a -Zdiscard-inactive-documents debug flag, which discards all inactive documents, even ones which are reachable through other same-origin pipelines.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14262.
These changes do not require tests because we should be able to use the existing tests with -Zdiscard-inactive-documents.

This change is

highfive · 2016-11-21T21:29:36Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/script_thread.rs, components/script/dom/document.rs
@KiChjang: components/script/script_thread.rs, components/script/dom/document.rs

highfive · 2016-11-21T21:29:37Z

Warning

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

asajeffrey · 2016-11-21T21:30:01Z

cc @Ms2ger @ConnorGBrewster @metajack

metajack · 2016-11-22T05:53:52Z

Maybe the name of that flag needs unsafe in it? It seems a little confusingly worded as is.

bors-servo · 2016-11-22T09:27:23Z

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

asajeffrey · 2016-11-22T14:28:58Z

@metajack: yes, that option name isn't my best-ever writing. -Zaggressively-discard-documents? -Zdiscard-on-navigation? -Zunsafe-navigation-discard?

bors-servo · 2016-11-23T00:01:21Z

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

asajeffrey · 2016-11-23T14:59:29Z

Might be worth disabling this by default, and enabling it with a flag?

metajack · 2016-11-23T16:01:35Z

@asajeffrey Why disable by default?

asajeffrey · 2016-11-23T16:06:06Z

@metajack just being conservative. This change makes servo a lot more like FF, in that back/fwd will almost always result in a reload. This is not necessarily bad, but it is different.

metajack · 2016-11-23T16:13:24Z

Leaving it off by default will make it consume infinite memory, no?

I can see a case for some middle ground setting being desireable, but I assume that is much more complex to guarantee since somehow we will have to keep the appropriate pipelines alive artificially.

asajeffrey · 2016-11-23T16:23:40Z

@metajack yes, there are more space leaks without this PR than with it. It wouldn't be too hard to implement whatever heuristic we want, we just add a message from the constellation to the script thread saying when a pipeline is discardable. I just piggybacked freeze/thaw because it was already there.

emilio · 2016-11-26T09:54:42Z

components/script/script_thread.rs

+/// The state that a document managed by this script thread could be in.
+#[must_root]
+enum DocumentState {
+    Kept(bool, JS<Document>),


Could you document what this bool means? I guess it's thawed, but would be ok to have it written here, or alternatively add a binary enum or a Kept { thawed: bool, document: JS<Document> } instead of a plain bool.

nox · 2016-11-27T11:15:56Z

components/script/script_thread.rs

+    fn trace(&self, trc: *mut JSTracer) {
+        // Note: we only trace thawed documents.
+        if let DocumentState::Kept(true, ref doc) = *self {
+            doc.trace(trc);


Why is this needed? Just derive the trait on DocumentState.

The derived version would trace documents even if they were Kept(false, doc). The idea is to implement poor-dev's weak references, that don't keep the documents alive.

KiChjang · 2016-12-01T00:45:54Z

r? @jdm

bors-servo · 2016-12-09T18:42:32Z

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

asajeffrey · 2017-01-04T17:10:12Z

@bors-servo try

bors-servo · 2017-01-04T17:10:18Z

⌛ Trying commit c14c431 with merge 9673fcb...


        Auto merge of #14312 - asajeffrey:script-discard-documents, r=<try>

Implement discarding Document objects to reclaim space.  This PR implements document discarding. Active documents are kept alive strongly, but inactive documents are only kept alive weakly. When a document is GCd, it is marked as discarded, and if it is every reactivated, a reload of the URL is triggered. Note that this PR is pretty aggressive about discarding, and can any inactive document (other than those being kept alive by other same-origin pipelines). We might want to damp it down a bit. Also note that this interacts with browser.html in that the reloading triggered by reactivating a document triggers mozbrowser events. To test this, I added a `-Zdiscard-inactive-documents` debug flag, which discards all inactive documents, even ones which are reachable through other same-origin pipelines. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14262. - [X] These changes do not require tests because we should be able to use the existing tests with `-Zdiscard-inactive-documents`.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14312)

bors-servo · 2017-01-04T18:17:23Z

☀️ 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-gnu-dev, windows-msvc-dev

jdm · 2017-01-04T18:51:34Z

This is ready to merge from my point of view.

asajeffrey · 2017-01-04T19:19:46Z

@bors-servo r=cbrewster cross fingers

bors-servo · 2017-01-04T19:19:47Z

📌 Commit c14c431 has been approved by cbrewster

cbrewster · 2017-01-04T21:45:54Z

@bors-servo retry

bors-servo · 2017-01-04T21:58:59Z

⌛ Testing commit c14c431 with merge 16b0da5...


        Auto merge of #14312 - asajeffrey:script-discard-documents, r=cbrewster

Implement discarding Document objects to reclaim space.  This PR implements document discarding. Active documents are kept alive strongly, but inactive documents are only kept alive weakly. When a document is GCd, it is marked as discarded, and if it is every reactivated, a reload of the URL is triggered. Note that this PR is pretty aggressive about discarding, and can any inactive document (other than those being kept alive by other same-origin pipelines). We might want to damp it down a bit. Also note that this interacts with browser.html in that the reloading triggered by reactivating a document triggers mozbrowser events. To test this, I added a `-Zdiscard-inactive-documents` debug flag, which discards all inactive documents, even ones which are reachable through other same-origin pipelines. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14262. - [X] These changes do not require tests because we should be able to use the existing tests with `-Zdiscard-inactive-documents`.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14312)

bors-servo · 2017-01-04T22:58:17Z

☀️ 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-gnu-dev, windows-msvc-dev


        Auto merge of #14860 - asajeffrey:constellation-index-past-correctly-…

…when-discarding, r=cbrewster Index the session past correctly when discarding.  Oops, indexed from the wrong end when discarding documents in #14312. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because we're not testing document discarding   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14860)


        Auto merge of #14860 - asajeffrey:constellation-index-past-correctly-…

…when-discarding, r=cbrewster Index the session past correctly when discarding.  Oops, indexed from the wrong end when discarding documents in #14312. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because we're not testing document discarding   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14860)


        Auto merge of #14860 - asajeffrey:constellation-index-past-correctly-…

…when-discarding, r=cbrewster Index the session past correctly when discarding.  Oops, indexed from the wrong end when discarding documents in #14312. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because we're not testing document discarding   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14860)

asajeffrey added A-content/script I-memory-leak labels Nov 21, 2016

highfive assigned KiChjang Nov 21, 2016

highfive added the S-awaiting-review label Nov 21, 2016

highfive added the S-needs-rebase label Nov 22, 2016

asajeffrey force-pushed the asajeffrey:script-discard-documents branch from e398d45 to 376f059 Nov 22, 2016

asajeffrey removed the S-needs-rebase label Nov 22, 2016

highfive added the S-needs-rebase label Nov 23, 2016

asajeffrey force-pushed the asajeffrey:script-discard-documents branch from 376f059 to 39f3cea Nov 23, 2016

asajeffrey removed the S-needs-rebase label Nov 23, 2016

emilio reviewed Nov 26, 2016

View changes

nox reviewed Nov 27, 2016

View changes

asajeffrey force-pushed the asajeffrey:script-discard-documents branch from 39f3cea to 4d70ef6 Nov 28, 2016

asajeffrey mentioned this pull request Nov 29, 2016

Discard documents and browsing contexts synchronously #14411

Closed

highfive assigned jdm and unassigned KiChjang Dec 1, 2016

cbrewster approved these changes Jan 4, 2017

View changes

highfive assigned cbrewster and unassigned jdm Jan 4, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 4, 2017

highfive removed the S-awaiting-merge label Jan 4, 2017

cbrewster mentioned this pull request Jan 5, 2017

assertion failed: self.suspended_since.get().is_some() #13716

Closed

asajeffrey mentioned this pull request Jan 5, 2017

Index the session past correctly when discarding. #14860

Merged

3 of 3 tasks complete

This was referenced Jan 6, 2017

Prune cached pipelines #6160

Closed

Servo leaks pages when traversing links #5364

Closed

What should the intermediate state of a reloaded document be? #14666

Closed

asajeffrey mentioned this pull request Jan 6, 2017

Save additional load data in Frame #14888

Open

cbrewster mentioned this pull request Jan 7, 2017

Add test to ensure the timestamp is the same after a reload #14908

Closed

jdm mentioned this pull request Jan 11, 2017

Convert max-session-history command line argument into a preference #14960

Closed

asajeffrey mentioned this pull request Jan 12, 2017

Constellation needs better origin handling #10968

Closed

servo / servo

Implement discarding Document objects to reclaim space. #14312

Implement discarding Document objects to reclaim space. #14312

asajeffrey commented Nov 21, 2016 •

edited by larsbergstrom

highfive commented Nov 21, 2016

highfive commented Nov 21, 2016

asajeffrey commented Nov 21, 2016

metajack commented Nov 22, 2016

bors-servo commented Nov 22, 2016

asajeffrey commented Nov 22, 2016

bors-servo commented Nov 23, 2016

asajeffrey commented Nov 23, 2016

metajack commented Nov 23, 2016

asajeffrey commented Nov 23, 2016

metajack commented Nov 23, 2016

asajeffrey commented Nov 23, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Dec 1, 2016

bors-servo commented Dec 9, 2016

asajeffrey commented Jan 4, 2017

bors-servo commented Jan 4, 2017

bors-servo commented Jan 4, 2017

jdm commented Jan 4, 2017

asajeffrey commented Jan 4, 2017

bors-servo commented Jan 4, 2017

cbrewster commented Jan 4, 2017

bors-servo commented Jan 4, 2017

bors-servo commented Jan 4, 2017

servo / servo

Join GitHub today

Implement discarding Document objects to reclaim space. #14312

Implement discarding Document objects to reclaim space. #14312

Conversation

asajeffrey commented Nov 21, 2016 • edited by larsbergstrom

highfive commented Nov 21, 2016

highfive commented Nov 21, 2016

asajeffrey commented Nov 21, 2016

metajack commented Nov 22, 2016

bors-servo commented Nov 22, 2016

asajeffrey commented Nov 22, 2016

bors-servo commented Nov 23, 2016

asajeffrey commented Nov 23, 2016

metajack commented Nov 23, 2016

asajeffrey commented Nov 23, 2016

metajack commented Nov 23, 2016

asajeffrey commented Nov 23, 2016

This comment has been minimized.

emilio Nov 26, 2016

This comment has been minimized.

asajeffrey Nov 28, 2016

This comment has been minimized.

nox Nov 27, 2016

This comment has been minimized.

asajeffrey Nov 28, 2016

KiChjang commented Dec 1, 2016

bors-servo commented Dec 9, 2016

asajeffrey commented Jan 4, 2017

bors-servo commented Jan 4, 2017

bors-servo commented Jan 4, 2017

jdm commented Jan 4, 2017

asajeffrey commented Jan 4, 2017

bors-servo commented Jan 4, 2017

cbrewster commented Jan 4, 2017

bors-servo commented Jan 4, 2017

bors-servo commented Jan 4, 2017

asajeffrey commented Nov 21, 2016 •

edited by larsbergstrom