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 upImplement microtask checkpoints #15189
Merged
Conversation
highfive
commented
Jan 25, 2017
|
Heads up! This PR modifies the following files:
|
|
Just some comments. |
| assert_eq!(&*entry.global as *const GlobalScope as usize, | ||
| self.global, | ||
| assert_eq!(&*entry.global as *const GlobalScope, | ||
| &*self.global as *const GlobalScope, |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jdm
Jan 25, 2017
Author
Member
The compiler complains that &GlobalScope doesn't implement the Debug trait if I get rid of the pointer casts.
This comment has been minimized.
This comment has been minimized.
nox
Jan 25, 2017
Member
Oh I see. I guess that's ok. Alternatively you can use assert!(... == ...); but that reports a less detailed panic.
components/script/microtask.rs
Outdated
|
|
||
| impl MicrotaskQueue { | ||
| /// Create a new PromiseJobQueue instance. | ||
| pub fn new() -> MicrotaskQueue { |
This comment has been minimized.
This comment has been minimized.
components/script/microtask.rs
Outdated
| } | ||
| // N.B. borrowing this vector is safe w.r.t. mutability, since any promise job that | ||
| // is enqueued while invoking these callbacks will be placed in `pending_queue`; | ||
| // `flushing_queue` is a static snapshot during this checkpoint. |
This comment has been minimized.
This comment has been minimized.
nox
Jan 25, 2017
Member
Why not just replace by an empty vector, like we do in Document::run_the_animation_frame_callbacks?
components/script/script_thread.rs
Outdated
| @@ -564,6 +565,13 @@ impl ScriptThreadFactory for ScriptThread { | |||
| } | |||
|
|
|||
| impl ScriptThread { | |||
| pub fn invoke_perform_a_microtask_checkpoint() { | |||
| SCRIPT_THREAD_ROOT.with(|root| { | |||
| let script_thread = root.as_ref().unwrap(); | |||
This comment has been minimized.
This comment has been minimized.
|
r? @nox |
|
Very nice work. |
| @@ -241,6 +241,7 @@ impl DedicatedWorkerGlobalScope { | |||
| break; | |||
| } | |||
| global.handle_event(event); | |||
| global.upcast::<WorkerGlobalScope>().perform_a_microtask_checkpoint(); | |||
This comment has been minimized.
This comment has been minimized.
| /// in FIFO order, synchronously, at some point in the future. | ||
| pub fn flush_promise_jobs(&self) { | ||
| /// Perform a microtask checkpoint. | ||
| pub fn perform_a_microtask_checkpoint(&self) { |
This comment has been minimized.
This comment has been minimized.
| /// Enqueue a promise callback for subsequent execution. | ||
| pub fn enqueue_promise_job(&self, job: EnqueuedPromiseCallback) { | ||
| /// Enqueue a microtask for subsequent execution. | ||
| pub fn enqueue_microtask(&self, job: Microtask) { |
This comment has been minimized.
This comment has been minimized.
| @@ -214,6 +214,7 @@ impl ServiceWorkerGlobalScope { | |||
| if !global.handle_event(event) { | |||
| break; | |||
| } | |||
| global.upcast::<WorkerGlobalScope>().perform_a_microtask_checkpoint(); | |||
This comment has been minimized.
This comment has been minimized.
| @@ -159,20 +158,12 @@ impl WorkerGlobalScope { | |||
| } | |||
| } | |||
|
|
|||
| pub fn enqueue_promise_job(&self, job: EnqueuedPromiseCallback) { | |||
| self.promise_job_queue.enqueue(job, self.upcast()); | |||
| pub fn enqueue_microtask(&self, job: Microtask) { | |||
This comment has been minimized.
This comment has been minimized.
|
|
||
| fn do_flush_promise_jobs(&self) { | ||
| self.promise_job_queue.flush_promise_jobs(|id| { | ||
| pub fn perform_a_microtask_checkpoint(&self) { |
This comment has been minimized.
This comment has been minimized.
components/script/microtask.rs
Outdated
|
|
||
| /// A collection of microtasks in FIFO order. | ||
| #[derive(JSTraceable, HeapSizeOf)] | ||
| pub struct MicrotaskQueue { |
This comment has been minimized.
This comment has been minimized.
| }); | ||
|
|
||
| // Step 5 | ||
| if !thread::panicking() && incumbent_global().is_none() { |
This comment has been minimized.
This comment has been minimized.
Ms2ger
Jan 25, 2017
Contributor
I don't know that this check is correct, mostly because I don't know what the correct check is.
| // TODO use a settings object rather than this element's document/window | ||
| // Step 2 | ||
| let document = document_from_node(self); | ||
| if !document.is_fully_active() || !document.is_scripting_enabled() { |
This comment has been minimized.
This comment has been minimized.
components/script/microtask.rs
Outdated
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| use dom::bindings::callback::ExceptionHandling; |
This comment has been minimized.
This comment has been minimized.
components/script/microtask.rs
Outdated
| pub struct MicrotaskQueue { | ||
| /// A snapshot of `microtask_queue` that was taken at the start of the microtask checkpoint. | ||
| /// Used to work around mutability errors when appending new microtasks while performing | ||
| /// a microtask checkpoint. |
This comment has been minimized.
This comment has been minimized.
Ms2ger
Jan 25, 2017
Contributor
Add a note that this would be a local variable if it wasn't for rooting concerns.
|
@bors-servo: try |
bors-servo
added a commit
that referenced
this pull request
Jan 25, 2017
Implement microtask checkpoints This generalizes the work previously done for Promise job callbacks. There is now a microtask queue that correctly processes all queued microtasks after each turn of the event loop, as well as after a scripted callback finishes executing, and after a classic script executes. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #4283 - [X] There are tests for these changes <!-- 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/15189) <!-- Reviewable:end -->
|
|
|
@bors-servo: try |
|
|
|
|
|
@bors-servo: try |
bors-servo
added a commit
that referenced
this pull request
Jan 25, 2017
Implement microtask checkpoints This generalizes the work previously done for Promise job callbacks. There is now a microtask queue that correctly processes all queued microtasks after each turn of the event loop, as well as after a scripted callback finishes executing, and after a classic script executes. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #4283 - [X] There are tests for these changes <!-- 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/15189) <!-- Reviewable:end -->
|
|
|
|
|
|
@creativcoder You may be interested in the changes I made to the ServiceWorker job queue implementation. |
|
Review comments addressed. All tests pass now. |
|
@jdm Thanks for notifying. :) I went through it. I'm currently working on remaining part of the Update algorithm, to fetch a service worker script and update it and the rest. Guess, they will be needing changes accordingly. |
|
@jdm You said you don't want to put links on |
|
Because they both delegate to a single method that has the link instead. Adding the link to places like that feels confusing to me. |
|
Fair enough. @bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Feb 3, 2017
Implement microtask checkpoints This generalizes the work previously done for Promise job callbacks. There is now a microtask queue that correctly processes all queued microtasks after each turn of the event loop, as well as after a scripted callback finishes executing, and after a classic script executes. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #4283 - [X] There are tests for these changes <!-- 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/15189) <!-- Reviewable:end -->
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
jdm commentedJan 25, 2017
•
edited by larsbergstrom
This generalizes the work previously done for Promise job callbacks. There is now a microtask queue that correctly processes all queued microtasks after each turn of the event loop, as well as after a scripted callback finishes executing, and after a classic script executes.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is