Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discard documents to reclaim resources #14262

Closed
asajeffrey opened this issue Nov 17, 2016 · 6 comments · Fixed by #14312
Closed

Discard documents to reclaim resources #14262

asajeffrey opened this issue Nov 17, 2016 · 6 comments · Fixed by #14312
Labels
A-constellation Involves the constellation A-content/script Related to the script thread I-memory-leak A memory leak has been observed.

Comments

@asajeffrey
Copy link
Member

The HTML spec allows documents to be discarded to reclaim resources (https://html.spec.whatwg.org/multipage/#discard-a-document). In particular, documents that aren't active may be discarded (https://html.spec.whatwg.org/multipage/browsers.html#the-session-history-of-browsing-contexts:discard-a-document).

#11893 contains a partial implementation.

cc @Ms2ger @jdm @ConnorGBrewster @metajack

@asajeffrey asajeffrey added A-constellation Involves the constellation A-content/script Related to the script thread I-memory-leak A memory leak has been observed. labels Nov 17, 2016
@asajeffrey
Copy link
Member Author

In https://html.spec.whatwg.org/multipage/#garbage-collection-and-browsing-contexts:discard-a-document:

User agents may discard top-level browsing contexts at any time (typically, in response to user requests, e.g. when a user force-closes a window containing one or more top-level browsing contexts). Other browsing contexts must be discarded once their WindowProxy object is eligible for garbage collection, in addition to the other places where this specification requires them to be discarded.

so if any pipeline P1 in a script thread contains a pointer to a DOM node from another pipeline P2 in the same script thread, then that DOM node will keep P2's WindowProxy alive, so P2's document cannot be discarded.

AFAICT, this either means we need to hook into the SM gc to work out when we can reclaim documents, or we need to discard an entire script thread at a time.

@asajeffrey
Copy link
Member Author

IRC conversation with @nox: http://logs.glob.uno/?c=mozilla%23servo&s=17+Nov+2016&e=17+Nov+2016#c563837

TL;DR we can make Document WeakReferenceable, and use that to hook into the gc.

@asajeffrey
Copy link
Member Author

My first shot at it: master...asajeffrey:constellation-script-discard-documents (not actually tested yet).

@asajeffrey
Copy link
Member Author

Hmm, just making Document weak referenceable (asajeffrey@a56684e) causes a panic:

assertion failed: self.is_object() (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(1) }, at /home/ajeffrey/github/asajeffrey/servo/.cargo/git/checkouts/rust-mozjs-8611526964119dd6/master/src/jsval.rs:373)
stack backtrace:
   0:     0x562c67470011 - backtrace::backtrace::libunwind::trace
                        at /home/ajeffrey/github/asajeffrey/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.2.3/src/backtrace/mod.rs:82
                         - backtrace::backtrace::trace<closure>
                        at /home/ajeffrey/github/asajeffrey/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.2.3/src/backtrace/mod.rs:70
   1:     0x562c67470cc8 - backtrace::capture::{{impl}}::new
                        at /home/ajeffrey/github/asajeffrey/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.2.3/src/lib.rs:96
   2:     0x562c63ea3d3c - servo::main::{{closure}}
                        at /home/ajeffrey/github/asajeffrey/servo/ports/servo/main.rs:120
   3:     0x562c68fcf1d4 - std::panicking::rust_panic_with_hook
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:452
   4:     0x562c6521b2f3 - std::panicking::begin_panic<&str>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:413
   5:     0x562c65da5d69 - js::jsval::{{impl}}::to_object
                        at /home/ajeffrey/github/asajeffrey/servo/ports/servo/<std macros>:3
                         - script::dom::bindings::proxyhandler::get_expando_object
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/dom/bindings/proxyhandler.rs:166
   6:     0x562c65da5f4c - script::dom::bindings::proxyhandler::ensure_expando_object
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/dom/bindings/proxyhandler.rs:176
   7:     0x562c663a5424 - script::dom::bindings::codegen::Bindings::DocumentBinding::DocumentBinding::Wrap
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/dom/bindings/mod.rs:160
   8:     0x562c65db5107 - script::dom::bindings::reflector::reflect_dom_object<script::dom::document::Document,script::dom::window::Window>
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/dom/bindings/reflector.rs:25
   9:     0x562c6610d120 - script::dom::document::{{impl}}::new
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/dom/document.rs:1880
  10:     0x562c662f79d3 - script::script_thread::{{impl}}::load
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:1721
  11:     0x562c66601d14 - script::script_thread::{{impl}}::handle_page_headers_available::{{closure}}
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:1441
  12:     0x562c650ba527 - core::option::{{impl}}::map<net_traits::Metadata,script::dom::bindings::js::Root<script::dom::servoparser::ServoParser>,closure>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libcore/option.rs:383
  13:     0x562c662f28e5 - script::script_thread::{{impl}}::handle_page_headers_available
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:1441
  14:     0x562c66643dd2 - script::script_thread::{{impl}}::page_headers_available::{{closure}}
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:561
  15:     0x562c65691fac - std::thread::local::{{impl}}::with<core::cell::Cell<core::option::Option<*const script::script_thread::ScriptThread>>,closure,core::option::Option<script::dom::bindings::js::Root<script::dom::servoparser::ServoParser>>>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/thread/local.rs:245
  16:     0x562c662df8f3 - script::script_thread::{{impl}}::page_headers_available
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:559
  17:     0x562c662382ae - script::dom::servoparser::{{impl}}::process_response
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/dom/servoparser/mod.rs:381
  18:     0x562c65b1e233 - net_traits::{{impl}}::process<script::dom::servoparser::ParserContext>
                        at /home/ajeffrey/github/asajeffrey/servo/components/net_traits/lib.rs:289
  19:     0x562c662d14ec - script::network_listener::{{impl}}::handler<net_traits::FetchResponseMsg,script::dom::servoparser::ParserContext>
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/network_listener.rs:71
  20:     0x562c662e7a78 - script::script_thread::{{impl}}::handle_msg_from_script
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:1010
  21:     0x562c665fb0c3 - script::script_thread::{{impl}}::handle_msgs::{{closure}}
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:841
  22:     0x562c662e5297 - script::script_thread::{{impl}}::profile_event<closure,core::option::Option<bool>>
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:932
  23:     0x562c662e3887 - script::script_thread::{{impl}}::handle_msgs
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:834
  24:     0x562c662e1d0e - script::script_thread::{{impl}}::start
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:691
  25:     0x562c665fa6e4 - script::script_thread::{{impl}}::create::{{closure}}::{{closure}}
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:544
  26:     0x562c64f4b305 - profile_traits::mem::{{impl}}::run_with_memory_reporting<closure,fn(profile_traits::mem::ReportsChan) -> script::script_runtime::CommonScriptMsg,script::script_runtime::CommonScriptMsg,std::sync::mpsc::Sender<script::script_thread::MainThreadScriptMsg>>
                        at /home/ajeffrey/github/asajeffrey/servo/components/profile_traits/mem.rs:63
  27:     0x562c667d5329 - script::script_thread::{{impl}}::create::{{closure}}
                        at /home/ajeffrey/github/asajeffrey/servo/components/script/script_thread.rs:543
  28:     0x562c65c3dcea - std::panic::{{impl}}::call_once<(),closure>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panic.rs:295
  29:     0x562c654e9a38 - std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:356
  30:     0x562c68fd783a - panic_unwind::__rust_maybe_catch_panic
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libpanic_unwind/lib.rs:97
  31:     0x562c653d8bb9 - std::panicking::try<(),std::panic::AssertUnwindSafe<closure>>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panicking.rs:332
  32:     0x562c651eba24 - std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/panic.rs:351
  33:     0x562c6661a40c - std::thread::{{impl}}::spawn::{{closure}}<closure,()>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/thread/mod.rs:278
  34:     0x562c65985969 - alloc::boxed::{{impl}}::call_box<(),closure>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/liballoc/boxed.rs:595
  35:     0x562c68fcd754 - alloc::boxed::{{impl}}::call_once<(),()>
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/liballoc/boxed.rs:605
                         - std::sys_common::thread::start_thread
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/sys/common/thread.rs:21
                         - std::sys::thread::{{impl}}::new::thread_start
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/libstd/sys/unix/thread.rs:84
  36:     0x7f770d9006a9 - start_thread
  37:     0x7f770d41f13c - __clone
  38:                0x0 - <unknown>

@asajeffrey
Copy link
Member Author

IRC conversation with @nox: http://logs.glob.uno/?c=mozilla%23servo&s=18+Nov+2016&e=18+Nov+2016#c564456

TL;DR: expando objects can't be weak referenceable atm.

@asajeffrey
Copy link
Member Author

I rewrote my implementation for this, it now just keeps track of discarded documents in script, the constellation is unchanged: https://github.com/asajeffrey/servo/commits/script-discard-documents

bors-servo pushed a commit that referenced this issue Dec 16, 2016
Implement discarding Document objects to reclaim space.

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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`.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14312)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Dec 16, 2016
Implement discarding Document objects to reclaim space.

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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`.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14312)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Dec 18, 2016
Implement discarding Document objects to reclaim space.

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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`.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14312)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 4, 2017
Implement discarding Document objects to reclaim space.

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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`.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14312)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 4, 2017
Implement discarding Document objects to reclaim space.

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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`.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14312)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-constellation Involves the constellation A-content/script Related to the script thread I-memory-leak A memory leak has been observed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant