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

style: Make WorkQueue creation fallible. #13041

Merged
merged 1 commit into from Aug 26, 2016
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

style: Make WorkQueue creation fallible.

Fixes bug 1290205 in bugzilla.
  • Loading branch information
emilio committed Aug 26, 2016
commit 4194ba063aeccd4a421399c8abc740ed95ab1cd7
@@ -399,7 +399,7 @@ impl LayoutThread {
MediaType::Screen,
opts::get().initial_window_size.to_f32() * ScaleFactor::new(1.0));
let parallel_traversal = if layout_threads != 1 {
Some(WorkQueue::new("LayoutWorker", thread_state::LAYOUT, layout_threads))
WorkQueue::new("LayoutWorker", thread_state::LAYOUT, layout_threads).ok()
} else {
None
};
@@ -18,8 +18,8 @@ use deque::{self, Abort, Data, Empty, Stealer, Worker};
use rand::{Rng, XorShiftRng, weak_rng};
use std::sync::atomic::{AtomicUsize, Ordering};
use std::sync::mpsc::{Receiver, Sender, channel};
use std::thread;
use thread_state;
use util::thread::spawn_named;

/// A unit of work.
///
@@ -244,10 +244,11 @@ impl<QueueData: Sync, WorkData: Send> WorkQueue<QueueData, WorkData> {
/// it.
pub fn new(thread_name: &'static str,
state: thread_state::ThreadState,
thread_count: usize) -> WorkQueue<QueueData, WorkData> {
thread_count: usize) -> Result<WorkQueue<QueueData, WorkData>, ()> {
// Set up data structures.
let (supervisor_chan, supervisor_port) = channel();
let (mut infos, mut threads) = (vec!(), vec!());
let mut infos = Vec::with_capacity(thread_count);
let mut threads = Vec::with_capacity(thread_count);
for i in 0..thread_count {
let (worker_chan, worker_port) = channel();
let (worker, thief) = deque::new();
@@ -276,21 +277,42 @@ impl<QueueData: Sync, WorkData: Send> WorkQueue<QueueData, WorkData> {
}

// Spawn threads.
let mut thread_handles = vec![];

This comment has been minimized.

@bholley

bholley Aug 26, 2016

Contributor

What is the difference between vec![] and vec!()?

This comment has been minimized.

@KiChjang

KiChjang Aug 26, 2016

Member

No difference, it's just personal style.

This comment has been minimized.

@bholley

bholley Aug 26, 2016

Contributor

Seems like we should specify one or the other in the style guide.

This comment has been minimized.

@emilio

emilio Aug 26, 2016

Author Member

I use to prefer vec![] because it mimics the style of arrays and slices (with the square brackets). But yeah, you can use whatever you want.

for (i, thread) in threads.into_iter().enumerate() {
spawn_named(
format!("{} worker {}/{}", thread_name, i + 1, thread_count),
move || {
let handle = thread::Builder::new()
.name(format!("{} worker {}/{}", thread_name, i + 1, thread_count))
.spawn(move || {
thread_state::initialize(state | thread_state::IN_WORKER);
let mut thread = thread;
thread.start()
})
});
match handle {
Ok(handle) => {
thread_handles.push(handle);
}
Err(err) => {
warn!("Failed spawning thread: {:?}", err);
break;
}
}
}

if thread_handles.len() != thread_count {
// At least one worker thread failed to be created, just close the
// rest of them, and return an error.
for (i, handle) in thread_handles.into_iter().enumerate() {
let _ = infos[i].chan.send(WorkerMsg::Exit);
let _ = handle.join();

This comment has been minimized.

@bholley

bholley Aug 26, 2016

Contributor

Could we make the thread handle vec an RAII class instead which automatically exits and joins the threads? That would allow us to return directly from the error case rather than doing the somewhat-wonky length check.

This comment has been minimized.

@emilio

emilio Aug 26, 2016

Author Member

Hm... Not really, or at least not in a way that's easier than this (because otherwise the threads will be joined when the function returns). Of course we could carry it with the queue, or adding a method to the handle wrapper so it detached it, but I don't think it's simpler than this.

}

return Err(());
}

WorkQueue {
Ok(WorkQueue {
workers: infos,
port: supervisor_port,
work_count: 0,
}
})
}

/// Enqueues a block into the work queue.
@@ -38,7 +38,7 @@ pub struct PerDocumentStyleData {
pub expired_animations: Arc<RwLock<HashMap<OpaqueNode, Vec<Animation>>>>,

// FIXME(bholley): This shouldn't be per-document.
pub work_queue: WorkQueue<SharedStyleContext, WorkQueueData>,
pub work_queue: Option<WorkQueue<SharedStyleContext, WorkQueueData>>,

pub num_threads: usize,
}
@@ -68,7 +68,11 @@ impl PerDocumentStyleData {
new_animations_receiver: new_anims_receiver,
running_animations: Arc::new(RwLock::new(HashMap::new())),
expired_animations: Arc::new(RwLock::new(HashMap::new())),
work_queue: WorkQueue::new("StyleWorker", thread_state::LAYOUT, *NUM_THREADS),
work_queue: if *NUM_THREADS <= 1 {
None
} else {
WorkQueue::new("StyleWorker", thread_state::LAYOUT, *NUM_THREADS).ok()
},
num_threads: *NUM_THREADS,
}
}
@@ -91,6 +95,8 @@ impl PerDocumentStyleData {

impl Drop for PerDocumentStyleData {
fn drop(&mut self) {
self.work_queue.shutdown();
if let Some(ref mut queue) = self.work_queue {
queue.shutdown();
}
}
}
@@ -103,11 +103,11 @@ fn restyle_subtree(node: GeckoNode, raw_data: *mut RawServoStyleSet) {

// We ensure this is true before calling Servo_RestyleSubtree()
debug_assert!(node.is_dirty() || node.has_dirty_descendants());
if per_doc_data.num_threads == 1 {
if per_doc_data.num_threads == 1 || per_doc_data.work_queue.is_none() {
sequential::traverse_dom::<GeckoNode, RecalcStyleOnly>(node, &shared_style_context);
} else {
parallel::traverse_dom::<GeckoNode, RecalcStyleOnly>(node, &shared_style_context,
&mut per_doc_data.work_queue);
per_doc_data.work_queue.as_mut().unwrap());
}
}

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