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

Implement discarding Document objects to reclaim space. #14312

Merged
merged 5 commits into from Jan 4, 2017
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

Implement discarding Document objects to reclaim space.

  • Loading branch information
asajeffrey committed Jan 4, 2017
commit 78c87ea8d7bf6cbc2e2a8ea8a27e764222baf43d
@@ -68,6 +68,9 @@ pub struct Opts {

pub output_file: Option<String>,

/// How much session history to keep in each tab.
pub max_session_history: usize,

/// Replace unpaires surrogates in DOM strings with U+FFFD.
/// See https://github.com/servo/servo/issues/6564
pub replace_surrogates: bool,
@@ -518,6 +521,7 @@ pub fn default_opts() -> Opts {
userscripts: None,
user_stylesheets: Vec::new(),
output_file: None,
max_session_history: 256,

This comment has been minimized.

@cbrewster

cbrewster Jan 3, 2017

Member

Out of curiosity, is there a reason the default is 256? That seems a bit large to me

This comment has been minimized.

@asajeffrey

asajeffrey Jan 4, 2017

Author Member

Number rather chosen at random. I'm not sure how to make a rational choice for this number, especially since the choice is probably very dependent on hardware: the main resource I expect we're saving is swap space (since frozen threads don't have timers so shouldn't be swapping themselves back in) which is very different, e.g. on a phone vs a PC.

replace_surrogates: false,
gc_profile: false,
load_webfonts_synchronously: false,
@@ -611,6 +615,7 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
"Probability of randomly closing a pipeline (for testing constellation hardening).",
"0.0");
opts.optopt("", "random-pipeline-closure-seed", "A fixed seed for repeatbility of random pipeline closure.", "");
opts.optopt("", "max-session-history", "Maximum amount of session history to store in each tab.", "256");
opts.optmulti("Z", "debug",
"A comma-separated string of debug options. Pass help to show available options.", "");
opts.optflag("h", "help", "Print this message");
@@ -779,6 +784,10 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
}
};

let max_session_history = opt_match.opt_str("max-session-history").map(|max| {
max.parse().unwrap_or_else(|err| args_fail(&format!("Error parsing option: --max-session-history ({})", err)))
}).unwrap_or(256);

This comment has been minimized.

@jdm

jdm Jan 3, 2017

Member

This feels like it might be better as a preference, rather than a command-line switch.

This comment has been minimized.

@asajeffrey

asajeffrey Jan 4, 2017

Author Member

Do we have a way to set a numeric preference from the command line? AFAICT we can only set strings and booleans. We use the command line for running the wpt suite with --binary-arg=--max-session-history=N.

This comment has been minimized.

@jdm

jdm Jan 4, 2017

Member

You're right; we should definitely add that ability.

This comment has been minimized.

@asajeffrey

asajeffrey Jan 4, 2017

Author Member

For the moment, I'm going to leave this alone then.


if opt_match.opt_present("M") {
MULTIPROCESS.store(true, Ordering::SeqCst)
}
@@ -820,6 +829,7 @@ pub fn from_cmdline_args(args: &[String]) -> ArgumentParsingResult {
userscripts: opt_match.opt_default("userscripts", ""),
user_stylesheets: user_stylesheets,
output_file: opt_match.opt_str("o"),
max_session_history: max_session_history,
replace_surrogates: debug_options.replace_surrogates,
gc_profile: debug_options.gc_profile,
load_webfonts_synchronously: debug_options.load_webfonts_synchronously,

Large diffs are not rendered by default.

@@ -8,6 +8,7 @@ use std::collections::HashMap;
use std::iter::once;
use std::mem::replace;
use std::time::Instant;
use servo_url::ServoUrl;

/// A frame in the frame tree.
/// Each frame is the constrellation's view of a browsing context.
@@ -22,43 +23,59 @@ pub struct Frame {
/// The frame id.
pub id: FrameId,

/// The timestamp for the current session history entry
pub instant: Instant,

/// The pipeline for the current session history entry
pub pipeline_id: PipelineId,

/// The URL for the current session history entry
pub url: ServoUrl,

/// The past session history, ordered chronologically.
pub prev: Vec<FrameState>,

/// The currently active session history entry.
pub current: FrameState,

/// The future session history, ordered reverse chronologically.
pub next: Vec<FrameState>,
}

impl Frame {
/// Create a new frame.
/// Note this just creates the frame, it doesn't add it to the frame tree.
pub fn new(id: FrameId, pipeline_id: PipelineId) -> Frame {
pub fn new(id: FrameId, pipeline_id: PipelineId, url: ServoUrl) -> Frame {
Frame {
id: id,
pipeline_id: pipeline_id,
instant: Instant::now(),
url: url,
prev: vec!(),
current: FrameState::new(pipeline_id, id),
next: vec!(),
}
}

/// Get the current frame state.
pub fn current(&self) -> FrameState {
FrameState {
instant: self.instant,
frame_id: self.id,
pipeline_id: Some(self.pipeline_id),
url: self.url.clone(),
}
}

/// Set the current frame entry, and push the current frame entry into the past.
pub fn load(&mut self, pipeline_id: PipelineId) {
self.prev.push(self.current.clone());
self.current = FrameState::new(pipeline_id, self.id);
pub fn load(&mut self, pipeline_id: PipelineId, url: ServoUrl) {
let current = self.current();
self.prev.push(current);
self.instant = Instant::now();
self.pipeline_id = pipeline_id;
self.url = url;
}

/// Set the future to be empty.
pub fn remove_forward_entries(&mut self) -> Vec<FrameState> {
replace(&mut self.next, vec!())
}

/// Set the current frame entry, and drop the current frame entry.
pub fn replace_current(&mut self, pipeline_id: PipelineId) -> FrameState {
replace(&mut self.current, FrameState::new(pipeline_id, self.id))
}
}

/// An entry in a frame's session history.
@@ -70,23 +87,18 @@ impl Frame {
pub struct FrameState {
/// The timestamp for when the session history entry was created
pub instant: Instant,
/// The pipeline for the document in the session history
pub pipeline_id: PipelineId,

/// The pipeline for the document in the session history,
/// None if the entry has been discarded
pub pipeline_id: Option<PipelineId>,

/// The URL for this entry, used to reload the pipeline if it has been discarded
pub url: ServoUrl,

/// The frame that this session history entry is part of
pub frame_id: FrameId,
}

impl FrameState {
/// Create a new session history entry.
fn new(pipeline_id: PipelineId, frame_id: FrameId) -> FrameState {
FrameState {
instant: Instant::now(),
pipeline_id: pipeline_id,
frame_id: frame_id,
}
}
}

/// Represents a pending change in the frame tree, that will be applied
/// once the new pipeline has loaded and completed initial layout / paint.
pub struct FrameChange {
@@ -100,9 +112,12 @@ pub struct FrameChange {
/// The pipeline for the document being loaded.
pub new_pipeline_id: PipelineId,

/// The URL for the document being loaded.
pub url: ServoUrl,

/// Is the new document replacing the current document (e.g. a reload)
/// or pushing it into the session history (e.g. a navigation)?
pub replace: bool,
pub replace: Option<FrameState>,
}

/// An iterator over a frame tree, returning the fully active frames in
@@ -137,14 +152,14 @@ impl<'a> Iterator for FrameTreeIterator<'a> {
continue;
},
};
let pipeline = match self.pipelines.get(&frame.current.pipeline_id) {
let pipeline = match self.pipelines.get(&frame.pipeline_id) {
Some(pipeline) => pipeline,
None => {
warn!("Pipeline {:?} iterated after closure.", frame.current.pipeline_id);
warn!("Pipeline {:?} iterated after closure.", frame.pipeline_id);
continue;
},
};
self.stack.extend(pipeline.children.iter().map(|&c| c));
self.stack.extend(pipeline.children.iter());
return Some(frame)
}
}
@@ -169,6 +184,7 @@ pub struct FullFrameTreeIterator<'a> {
impl<'a> Iterator for FullFrameTreeIterator<'a> {
type Item = &'a Frame;
fn next(&mut self) -> Option<&'a Frame> {
let pipelines = self.pipelines;
loop {
let frame_id = match self.stack.pop() {
Some(frame_id) => frame_id,
@@ -181,11 +197,12 @@ impl<'a> Iterator for FullFrameTreeIterator<'a> {
continue;
},
};
for entry in frame.prev.iter().chain(frame.next.iter()).chain(once(&frame.current)) {
if let Some(pipeline) = self.pipelines.get(&entry.pipeline_id) {
self.stack.extend(pipeline.children.iter().map(|&c| c));
}
}
let child_frame_ids = frame.prev.iter().chain(frame.next.iter())
.filter_map(|entry| entry.pipeline_id)
.chain(once(frame.pipeline_id))
.filter_map(|pipeline_id| pipelines.get(&pipeline_id))
.flat_map(|pipeline| pipeline.children.iter());
self.stack.extend(child_frame_ids);
return Some(frame)
}
}
@@ -3,6 +3,7 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

#![feature(box_syntax)]
#![feature(conservative_impl_trait)]
#![feature(mpsc_select)]
#![feature(plugin)]
#![feature(proc_macro)]
@@ -901,18 +901,24 @@ impl Window {
}

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

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

self.current_state.set(WindowState::Zombie);
*self.js_runtime.borrow_mut() = None;
@@ -1445,6 +1451,12 @@ impl Window {
None
}

pub fn freeze(&self) {
self.upcast::<GlobalScope>().suspend();
// A hint to the JS runtime that now would be a good time to GC this window.
self.Gc();

This comment has been minimized.

@jdm

jdm Jan 3, 2017

Member

This window will not be GCed because it is being accessed by a root that will prevent it being GCed. I'm not sure that this hint makes sense here.

This comment has been minimized.

@asajeffrey

asajeffrey Jan 4, 2017

Author Member

We are GCing user script data and unattached DOM nodes here. Attached DOM nodes can't be reclaimed.

This comment has been minimized.

@jdm

jdm Jan 4, 2017

Member

I guess I read "GC this window" as "GC this particular window object."

This comment has been minimized.

@asajeffrey

asajeffrey Jan 4, 2017

Author Member

Good point, the comment isn't as clear as it could be. I'll fix that.

}

pub fn thaw(&self) {
self.upcast::<GlobalScope>().resume();

@@ -1346,7 +1346,7 @@ impl ScriptThread {
fn handle_freeze_msg(&self, id: PipelineId) {
let window = self.documents.borrow().find_window(id);
if let Some(window) = window {
window.upcast::<GlobalScope>().suspend();
window.freeze();
return;
}
let mut loads = self.incomplete_loads.borrow_mut();
@@ -228,18 +228,18 @@ impl OneshotTimers {
}

pub fn suspend(&self) {
assert!(self.suspended_since.get().is_none());
// Suspend is idempotent: do nothing if the timers are already suspended.

This comment has been minimized.

@cbrewster

cbrewster Jan 3, 2017

Member

While I think making freeze/thaw/suspend idempotent isn't a bad idea, I think it can let some issues fall through the cracks. For example #13716 is caused by a some Pipelines not being frozen when navigated from. We would have no idea that this was happening if suspend was idempotent. At a minimum, it might be nice to add a warn! when returning early here and in resume

This comment has been minimized.

@asajeffrey

asajeffrey Jan 4, 2017

Author Member

Added warnings.

if self.suspended_since.get().is_some() { return; }

self.suspended_since.set(Some(precise_time_ms()));
self.invalidate_expected_event_id();
}

pub fn resume(&self) {
assert!(self.suspended_since.get().is_some());

// Suspend is idempotent: do nothing if the timers are already suspended.
let additional_offset = match self.suspended_since.get() {
Some(suspended_since) => precise_time_ms() - suspended_since,
None => panic!("Timers are not suspended.")
None => return,
};

self.suspension_offset.set(self.suspension_offset.get() + additional_offset);
@@ -24,7 +24,7 @@ use std::path::Path;
use std::sync::Arc;
use url::{Url, Origin, Position};

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

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.