Implement discarding Document objects to reclaim space. by asajeffrey · Pull Request #14312 · servo/servo

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 Discard documents to reclaim resources #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:26Z

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:12Z

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-04T16:58:03Z

An IRC conversation with @jdm, @Ms2ger and @cbrewster: http://logs.glob.uno/?c=mozilla%23servo&s=4+Jan+2017&e=4+Jan+2017#c586779

TL;DR: this crash is probably caused by us not doing the window proxy to window mapping quite right, since we are generating a new BrowsingContext object for every Window.

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

@bors-servo try

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

⌛ Trying commit c14c431 with merge 9673fcb...

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...

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

asajeffrey added A-content/script Related to the script thread I-memory-leak A memory leak has been observed. labels Nov 21, 2016

highfive assigned KiChjang Nov 21, 2016

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

highfive added the S-needs-rebase There are merge conflict errors. label Nov 22, 2016

asajeffrey force-pushed the script-discard-documents branch from e398d45 to 376f059 Compare November 22, 2016 14:29

asajeffrey removed the S-needs-rebase There are merge conflict errors. label Nov 22, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Nov 23, 2016

asajeffrey force-pushed the script-discard-documents branch from 376f059 to 39f3cea Compare November 23, 2016 14:59

asajeffrey removed the S-needs-rebase There are merge conflict errors. label Nov 23, 2016

emilio reviewed Nov 26, 2016

View reviewed changes

nox reviewed Nov 27, 2016

View reviewed changes

asajeffrey force-pushed the script-discard-documents branch from 39f3cea to 4d70ef6 Compare November 28, 2016 17:15

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

asajeffrey mentioned this pull request Jan 4, 2017

Allow setting numeric preferences from the command line #14842

Closed

asajeffrey mentioned this pull request Jan 4, 2017

Don't create a new BrowsingContext object for each Window #14843

Closed

Default --max-session-history is now 16.

c14c431

cbrewster approved these changes Jan 4, 2017

View reviewed changes

highfive assigned cbrewster and unassigned jdm Jan 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 Jan 4, 2017

bors-servo merged commit c14c431 into servo:master Jan 4, 2017

cbrewster mentioned this pull request Jan 5, 2017

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

Closed

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

Uh oh!

Conversation

asajeffrey commented Nov 21, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Nov 21, 2016

Uh oh!

highfive commented Nov 21, 2016

Uh oh!

asajeffrey commented Nov 21, 2016

Uh oh!

metajack commented Nov 22, 2016

Uh oh!

bors-servo commented Nov 22, 2016

Uh oh!

asajeffrey commented Nov 22, 2016

Uh oh!

bors-servo commented Nov 23, 2016

Uh oh!

asajeffrey commented Nov 23, 2016

Uh oh!

metajack commented Nov 23, 2016

Uh oh!

asajeffrey commented Nov 23, 2016

Uh oh!

metajack commented Nov 23, 2016

Uh oh!

asajeffrey commented Nov 23, 2016

Uh oh!

emilio Nov 26, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

nox Nov 27, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Nov 28, 2016

Choose a reason for hiding this comment

Uh oh!

KiChjang commented Dec 1, 2016

Uh oh!

bors-servo commented Dec 9, 2016

Uh oh!

asajeffrey commented Jan 4, 2017

Uh oh!

asajeffrey commented Jan 4, 2017

Uh oh!

bors-servo commented Jan 4, 2017

Uh oh!

bors-servo commented Jan 4, 2017

Uh oh!

jdm commented Jan 4, 2017

Uh oh!

asajeffrey commented Jan 4, 2017

Uh oh!

bors-servo commented Jan 4, 2017

Uh oh!

cbrewster commented Jan 4, 2017

Uh oh!

bors-servo commented Jan 4, 2017

Uh oh!

bors-servo commented Jan 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

asajeffrey commented Nov 21, 2016 •

edited by larsbergstrom

Loading