Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upstyle: Make WorkQueue creation fallible. #13041
Conversation
highfive
commented
Aug 25, 2016
|
Heads up! This PR modifies the following files:
|
highfive
commented
Aug 25, 2016
|
|
|
r? @bholley or @SimonSapin |
| @@ -276,21 +277,42 @@ impl<QueueData: Sync, WorkData: Send> WorkQueue<QueueData, WorkData> { | |||
| } | |||
|
|
|||
| // Spawn threads. | |||
| let mut thread_handles = vec![]; | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
| // 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.
This comment has been minimized.
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.
This comment has been minimized.
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.
| @@ -68,7 +68,7 @@ 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: WorkQueue::new("StyleWorker", thread_state::LAYOUT, *NUM_THREADS).ok(), | |||
This comment has been minimized.
This comment has been minimized.
bholley
Aug 26, 2016
Contributor
While you're here, can we also avoid spawning the queue if num_threads is 1?
|
You rock. :-) r=me with comments addressed. @bors-servo delegate+ |
|
|
Fixes bug 1290205 in bugzilla.
|
@bors-servo: r=bholley |
|
|
style: Make WorkQueue creation fallible. <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [x] These changes do not require tests. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Fixes bug 1290205 in bugzilla. <!-- 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/13041) <!-- Reviewable:end -->
|
|
|
@bors-servo retry #8769 |
|
|
|
|
emilio commentedAug 25, 2016
•
edited by larsbergstrom
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsFixes bug 1290205 in bugzilla.
This change is