Skip to content

Commit 9673fcb

Browse files
author
bors-servo
authored
Auto merge of #14312 - asajeffrey:script-discard-documents, r=<try>
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 -->
2 parents 384e905 + c14c431 commit 9673fcb

File tree

8 files changed

+307
-219
lines changed

8 files changed

+307
-219
lines changed

‎components/config/opts.rs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,9 @@ pub struct Opts {
6868

6969
pub output_file: Option<String>,
7070

71+
/// How much session history to keep in each tab.
72+
pub max_session_history: usize,
73+
7174
/// Replace unpaires surrogates in DOM strings with U+FFFD.
7275
/// See https://github.com/servo/servo/issues/6564
7376
pub replace_surrogates: bool,
@@ -518,6 +521,7 @@ pub fn default_opts() -> Opts {
518521
userscripts: None,
519522
user_stylesheets: Vec::new(),
520523
output_file: None,
524+
max_session_history: 16,
521525
replace_surrogates: false,
522526
gc_profile: false,
523527
load_webfonts_synchronously: false,
@@ -611,6 +615,7 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
611615
"Probability of randomly closing a pipeline (for testing constellation hardening).",
612616
"0.0");
613617
opts.optopt("", "random-pipeline-closure-seed", "A fixed seed for repeatbility of random pipeline closure.", "");
618+
opts.optopt("", "max-session-history", "Maximum amount of session history to store in each tab.", "16");
614619
opts.optmulti("Z", "debug",
615620
"A comma-separated string of debug options. Pass help to show available options.", "");
616621
opts.optflag("h", "help", "Print this message");
@@ -779,6 +784,10 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
779784
}
780785
};
781786

787+
let max_session_history = opt_match.opt_str("max-session-history").map(|max| {
788+
max.parse().unwrap_or_else(|err| args_fail(&format!("Error parsing option: --max-session-history ({})", err)))
789+
}).unwrap_or(16);
790+
782791
if opt_match.opt_present("M") {
783792
MULTIPROCESS.store(true, Ordering::SeqCst)
784793
}
@@ -820,6 +829,7 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
820829
userscripts: opt_match.opt_default("userscripts", ""),
821830
user_stylesheets: user_stylesheets,
822831
output_file: opt_match.opt_str("o"),
832+
max_session_history: max_session_history,
823833
replace_surrogates: debug_options.replace_surrogates,
824834
gc_profile: debug_options.gc_profile,
825835
load_webfonts_synchronously: debug_options.load_webfonts_synchronously,

‎components/constellation/constellation.rs‎

Lines changed: 212 additions & 169 deletions
Large diffs are not rendered by default.

‎components/constellation/frame.rs‎

Lines changed: 52 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use msg::constellation_msg::{FrameId, PipelineId};
66
use pipeline::Pipeline;
7+
use servo_url::ServoUrl;
78
use std::collections::HashMap;
89
use std::iter::once;
910
use std::mem::replace;
@@ -22,43 +23,59 @@ pub struct Frame {
2223
/// The frame id.
2324
pub id: FrameId,
2425

26+
/// The timestamp for the current session history entry
27+
pub instant: Instant,
28+
29+
/// The pipeline for the current session history entry
30+
pub pipeline_id: PipelineId,
31+
32+
/// The URL for the current session history entry
33+
pub url: ServoUrl,
34+
2535
/// The past session history, ordered chronologically.
2636
pub prev: Vec<FrameState>,
2737

28-
/// The currently active session history entry.
29-
pub current: FrameState,
30-
3138
/// The future session history, ordered reverse chronologically.
3239
pub next: Vec<FrameState>,
3340
}
3441

3542
impl Frame {
3643
/// Create a new frame.
3744
/// Note this just creates the frame, it doesn't add it to the frame tree.
38-
pub fn new(id: FrameId, pipeline_id: PipelineId) -> Frame {
45+
pub fn new(id: FrameId, pipeline_id: PipelineId, url: ServoUrl) -> Frame {
3946
Frame {
4047
id: id,
48+
pipeline_id: pipeline_id,
49+
instant: Instant::now(),
50+
url: url,
4151
prev: vec!(),
42-
current: FrameState::new(pipeline_id, id),
4352
next: vec!(),
4453
}
4554
}
4655

56+
/// Get the current frame state.
57+
pub fn current(&self) -> FrameState {
58+
FrameState {
59+
instant: self.instant,
60+
frame_id: self.id,
61+
pipeline_id: Some(self.pipeline_id),
62+
url: self.url.clone(),
63+
}
64+
}
65+
4766
/// Set the current frame entry, and push the current frame entry into the past.
48-
pub fn load(&mut self, pipeline_id: PipelineId) {
49-
self.prev.push(self.current.clone());
50-
self.current = FrameState::new(pipeline_id, self.id);
67+
pub fn load(&mut self, pipeline_id: PipelineId, url: ServoUrl) {
68+
let current = self.current();
69+
self.prev.push(current);
70+
self.instant = Instant::now();
71+
self.pipeline_id = pipeline_id;
72+
self.url = url;
5173
}
5274

5375
/// Set the future to be empty.
5476
pub fn remove_forward_entries(&mut self) -> Vec<FrameState> {
5577
replace(&mut self.next, vec!())
5678
}
57-
58-
/// Set the current frame entry, and drop the current frame entry.
59-
pub fn replace_current(&mut self, pipeline_id: PipelineId) -> FrameState {
60-
replace(&mut self.current, FrameState::new(pipeline_id, self.id))
61-
}
6279
}
6380

6481
/// An entry in a frame's session history.
@@ -70,23 +87,18 @@ impl Frame {
7087
pub struct FrameState {
7188
/// The timestamp for when the session history entry was created
7289
pub instant: Instant,
73-
/// The pipeline for the document in the session history
74-
pub pipeline_id: PipelineId,
90+
91+
/// The pipeline for the document in the session history,
92+
/// None if the entry has been discarded
93+
pub pipeline_id: Option<PipelineId>,
94+
95+
/// The URL for this entry, used to reload the pipeline if it has been discarded
96+
pub url: ServoUrl,
97+
7598
/// The frame that this session history entry is part of
7699
pub frame_id: FrameId,
77100
}
78101

79-
impl FrameState {
80-
/// Create a new session history entry.
81-
fn new(pipeline_id: PipelineId, frame_id: FrameId) -> FrameState {
82-
FrameState {
83-
instant: Instant::now(),
84-
pipeline_id: pipeline_id,
85-
frame_id: frame_id,
86-
}
87-
}
88-
}
89-
90102
/// Represents a pending change in the frame tree, that will be applied
91103
/// once the new pipeline has loaded and completed initial layout / paint.
92104
pub struct FrameChange {
@@ -100,9 +112,12 @@ pub struct FrameChange {
100112
/// The pipeline for the document being loaded.
101113
pub new_pipeline_id: PipelineId,
102114

115+
/// The URL for the document being loaded.
116+
pub url: ServoUrl,
117+
103118
/// Is the new document replacing the current document (e.g. a reload)
104119
/// or pushing it into the session history (e.g. a navigation)?
105-
pub replace: bool,
120+
pub replace: Option<FrameState>,
106121
}
107122

108123
/// An iterator over a frame tree, returning the fully active frames in
@@ -137,14 +152,14 @@ impl<'a> Iterator for FrameTreeIterator<'a> {
137152
continue;
138153
},
139154
};
140-
let pipeline = match self.pipelines.get(&frame.current.pipeline_id) {
155+
let pipeline = match self.pipelines.get(&frame.pipeline_id) {
141156
Some(pipeline) => pipeline,
142157
None => {
143-
warn!("Pipeline {:?} iterated after closure.", frame.current.pipeline_id);
158+
warn!("Pipeline {:?} iterated after closure.", frame.pipeline_id);
144159
continue;
145160
},
146161
};
147-
self.stack.extend(pipeline.children.iter().map(|&c| c));
162+
self.stack.extend(pipeline.children.iter());
148163
return Some(frame)
149164
}
150165
}
@@ -169,6 +184,7 @@ pub struct FullFrameTreeIterator<'a> {
169184
impl<'a> Iterator for FullFrameTreeIterator<'a> {
170185
type Item = &'a Frame;
171186
fn next(&mut self) -> Option<&'a Frame> {
187+
let pipelines = self.pipelines;
172188
loop {
173189
let frame_id = match self.stack.pop() {
174190
Some(frame_id) => frame_id,
@@ -181,11 +197,12 @@ impl<'a> Iterator for FullFrameTreeIterator<'a> {
181197
continue;
182198
},
183199
};
184-
for entry in frame.prev.iter().chain(frame.next.iter()).chain(once(&frame.current)) {
185-
if let Some(pipeline) = self.pipelines.get(&entry.pipeline_id) {
186-
self.stack.extend(pipeline.children.iter().map(|&c| c));
187-
}
188-
}
200+
let child_frame_ids = frame.prev.iter().chain(frame.next.iter())
201+
.filter_map(|entry| entry.pipeline_id)
202+
.chain(once(frame.pipeline_id))
203+
.filter_map(|pipeline_id| pipelines.get(&pipeline_id))
204+
.flat_map(|pipeline| pipeline.children.iter());
205+
self.stack.extend(child_frame_ids);
189206
return Some(frame)
190207
}
191208
}

‎components/constellation/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

55
#![feature(box_syntax)]
6+
#![feature(conservative_impl_trait)]
67
#![feature(mpsc_select)]
78
#![feature(plugin)]
89
#![feature(proc_macro)]

‎components/script/dom/window.rs‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -901,18 +901,24 @@ impl Window {
901901
}
902902

903903
pub fn clear_js_runtime(&self) {
904+
// We tear down the active document, which causes all the attached
905+
// nodes to dispose of their layout data. This messages the layout
906+
// thread, informing it that it can safely free the memory.
904907
self.Document().upcast::<Node>().teardown();
905908

906-
// The above code may not catch all DOM objects
907-
// (e.g. DOM objects removed from the tree that haven't
908-
// been collected yet). Forcing a GC here means that
909-
// those DOM objects will be able to call dispose()
910-
// to free their layout data before the layout thread
911-
// exits. Without this, those remaining objects try to
912-
// send a message to free their layout data to the
913-
// layout thread when the script thread is dropped,
914-
// which causes a panic!
909+
// The above code may not catch all DOM objects (e.g. DOM
910+
// objects removed from the tree that haven't been collected
911+
// yet). There should not be any such DOM nodes with layout
912+
// data, but if there are, then when they are dropped, they
913+
// will attempt to send a message to the closed layout thread.
914+
// This causes memory safety issues, because the DOM node uses
915+
// the layout channel from its window, and the window has
916+
// already been GC'd. For nodes which do not have a live
917+
// pointer, we can avoid this by GCing now:
915918
self.Gc();
919+
// but there may still be nodes being kept alive by user
920+
// script.
921+
// TODO: ensure that this doesn't happen!
916922

917923
self.current_state.set(WindowState::Zombie);
918924
*self.js_runtime.borrow_mut() = None;
@@ -1445,6 +1451,15 @@ impl Window {
14451451
None
14461452
}
14471453

1454+
pub fn freeze(&self) {
1455+
self.upcast::<GlobalScope>().suspend();
1456+
// A hint to the JS runtime that now would be a good time to
1457+
// GC any unreachable objects generated by user script,
1458+
// or unattached DOM nodes. Attached DOM nodes can't be GCd yet,
1459+
// as the document might be thawed later.
1460+
self.Gc();
1461+
}
1462+
14481463
pub fn thaw(&self) {
14491464
self.upcast::<GlobalScope>().resume();
14501465

‎components/script/script_thread.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1346,7 +1346,7 @@ impl ScriptThread {
13461346
fn handle_freeze_msg(&self, id: PipelineId) {
13471347
let window = self.documents.borrow().find_window(id);
13481348
if let Some(window) = window {
1349-
window.upcast::<GlobalScope>().suspend();
1349+
window.freeze();
13501350
return;
13511351
}
13521352
let mut loads = self.incomplete_loads.borrow_mut();

‎components/script/timers.rs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -228,18 +228,20 @@ impl OneshotTimers {
228228
}
229229

230230
pub fn suspend(&self) {
231-
assert!(self.suspended_since.get().is_none());
231+
// Suspend is idempotent: do nothing if the timers are already suspended.
232+
if self.suspended_since.get().is_some() {
233+
return warn!("Suspending an already suspended timer.");
234+
}
232235

233236
self.suspended_since.set(Some(precise_time_ms()));
234237
self.invalidate_expected_event_id();
235238
}
236239

237240
pub fn resume(&self) {
238-
assert!(self.suspended_since.get().is_some());
239-
241+
// Suspend is idempotent: do nothing if the timers are already suspended.
240242
let additional_offset = match self.suspended_since.get() {
241243
Some(suspended_since) => precise_time_ms() - suspended_since,
242-
None => panic!("Timers are not suspended.")
244+
None => return warn!("Resuming an already resumed timer."),
243245
};
244246

245247
self.suspension_offset.set(self.suspension_offset.get() + additional_offset);

‎components/url/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ use std::path::Path;
2424
use std::sync::Arc;
2525
use url::{Url, Origin, Position};
2626

27-
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
27+
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
2828
#[cfg_attr(feature = "servo", derive(HeapSizeOf, Serialize, Deserialize))]
2929
pub struct ServoUrl(Arc<Url>);
3030

0 commit comments

Comments
 (0)