Use microtasks to await a stable state. #16861

gterzian · 2017-05-14T23:28:45Z

@jdm @KiChjang First pass at using microtasks to await a stable state. I ran into all sorts of problems to get it to compile, I think it's mainly related to the fact that the microtasks are stored in a Vec, which meant the Runnalbe.handler(self: Box<Self>) couldn't be called while iterating over the Vec... It compiles now although I haven't run any tests. I'm assuming I'm missing something fundamental and was hoping my changes would highlight the problems I run into, and you had a better idea of how to implement this... Perhaps we shouldn't pass a Runnable to await_stable_state at all?

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15375 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-05-14T23:28:50Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/script_thread.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/bindings/trace.rs, components/script/microtask.rs
@KiChjang: components/script/script_thread.rs, components/script/dom/htmlmediaelement.rs, components/script/dom/bindings/trace.rs, components/script/microtask.rs

highfive · 2017-05-14T23:28:51Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

gterzian · 2017-05-15T07:32:22Z

As an update on the above, I have used this commit(as well as implemented a ref_handler method to the Runnable used) in #16238, and it all the tests in the-img-element passed succesfully. So as a proof of concept it works, however I am wondering if instead of adding this ref_handler method to Runnable, we should perhaps add a new Microtask specific trait.

jdm · 2017-05-15T13:57:16Z

components/script/dom/bindings/trace.rs

@@ -420,6 +421,13 @@ unsafe impl JSTraceable for Box<LayoutRPC + Send + 'static> {
    }
 }

+unsafe impl JSTraceable for Box<Runnable + Send + 'static> {


This is not a safe construct, since it allows us to store non-GC safe values in Runnables.

jdm · 2017-05-15T13:57:16Z

components/script/microtask.rs

@@ -28,6 +32,13 @@ pub struct MicrotaskQueue {
 #[derive(JSTraceable, HeapSizeOf)]
 pub enum Microtask {
    Promise(EnqueuedPromiseCallback),
+    AwaitStableState(Box<Runnable + Send>)


Instead of using Runnables for this and await_stable_state, let's have MedieElement(MediaElementMicrotask), ImageElement(ImageElementMicrotask), where the inner types are enums. We can define a MicrotaskRunnable trait that the enums implement with a handler(&self) method.

gterzian · 2017-05-16T14:05:53Z

@jdm thanks for the suggestions, I've made an attempt using enums...

Note I've had to impl HeapSizeOf for Trusted<HTMLMediaElement>, is it necessary to store the HTMLMediaElement inside a Trusted, since microtask are not multi-threaded as I understand it?

jdm · 2017-05-16T14:18:34Z

It's better to use #[ignore_heap_size_of = "Can't measure Trusted<T>"] instead.

jdm · 2017-05-16T14:19:24Z

As for whether it's necessary, that's a good point. It's true that there's no threading involved, so a Root value should work instead.

jdm · 2017-05-16T14:21:06Z

components/script/dom/htmlmediaelement.rs

+    }
+}
+
+impl HeapSizeOf for Trusted<HTMLMediaElement> {


Remove this.

jdm · 2017-05-16T14:21:06Z

components/script/script_thread.rs

@@ -626,14 +626,12 @@ impl ScriptThread {
    }

    // https://html.spec.whatwg.org/multipage/#await-a-stable-state
-    pub fn await_stable_state<T: Runnable + Send + 'static>(task: T) {
+    pub fn await_stable_state(task: Microtask) {
        //TODO use microtasks when they exist


Let's remove this comment :)

jdm · 2017-05-16T14:21:06Z

components/script/dom/htmlmediaelement.rs

+        base_url: ServoUrl
+    },
+    PauseIfNotInDocumentTask {
+        elem: Trusted<HTMLMediaElement>,


We can use Root<HTMLMediaElement> values now.

gterzian · 2017-05-17T00:55:40Z

@jdm thanks, ready for another round or review...

jdm · 2017-05-17T01:22:36Z

@bors-servo: r+
Thanks!

bors-servo · 2017-05-17T01:22:36Z

📋 Looks like this PR is still in progress, ignoring approval


        Auto merge of #16861 - gterzian:use_microtask_to_await_stable_state, …

@jdm

…r=jdm Use microtasks to await a stable state.  @jdm @KiChjang First pass at using microtasks to await a stable state. I ran into all sorts of problems to get it to compile, I think it's mainly related to the fact that the microtasks are stored in a `Vec`, which meant the `Runnalbe.handler(self: Box<Self>)` couldn't be called while iterating over the Vec... It compiles now although I haven't run any tests. I'm assuming I'm missing something fundamental and was hoping my changes would highlight the problems I run into, and you had a better idea of how to implement this... Perhaps we shouldn't pass a `Runnable` to `await_stable_state` at all? ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #15375 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16861)

bors-servo · 2017-05-19T04:49:28Z

💔 Test failed - linux-rel-wpt

jdm · 2017-05-19T05:21:27Z

Oh, it does seem like networkState_during_progress.html consistently times out. Could you revert that expectation change?


        use microtasks to await a stable state

gterzian · 2017-05-19T10:10:27Z

@jdm yep, done. Build failure seems unrelated by the way...

jdm · 2017-05-19T13:16:21Z

@bors-servo: r+

bors-servo · 2017-05-19T13:16:21Z

📌 Commit a8390ae has been approved by jdm

bors-servo · 2017-05-19T13:54:51Z

⌛ Testing commit a8390ae with merge f713c76...


        Auto merge of #16861 - gterzian:use_microtask_to_await_stable_state, …

@jdm

…r=jdm Use microtasks to await a stable state.  @jdm @KiChjang First pass at using microtasks to await a stable state. I ran into all sorts of problems to get it to compile, I think it's mainly related to the fact that the microtasks are stored in a `Vec`, which meant the `Runnalbe.handler(self: Box<Self>)` couldn't be called while iterating over the Vec... It compiles now although I haven't run any tests. I'm assuming I'm missing something fundamental and was hoping my changes would highlight the problems I run into, and you had a better idea of how to implement this... Perhaps we shouldn't pass a `Runnable` to `await_stable_state` at all? ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #15375 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16861)

bors-servo · 2017-05-19T15:44:53Z

💔 Test failed - linux-rel-wpt

jdm · 2017-05-19T15:58:34Z

@bors-servo: retry

the builder was totally busted

bors-servo · 2017-05-20T01:09:56Z

⌛ Testing commit a8390ae with merge f054911...


        Auto merge of #16861 - gterzian:use_microtask_to_await_stable_state, …

@jdm

…r=jdm Use microtasks to await a stable state.  @jdm @KiChjang First pass at using microtasks to await a stable state. I ran into all sorts of problems to get it to compile, I think it's mainly related to the fact that the microtasks are stored in a `Vec`, which meant the `Runnalbe.handler(self: Box<Self>)` couldn't be called while iterating over the Vec... It compiles now although I haven't run any tests. I'm assuming I'm missing something fundamental and was hoping my changes would highlight the problems I run into, and you had a better idea of how to implement this... Perhaps we shouldn't pass a `Runnable` to `await_stable_state` at all? ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #15375 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16861)

bors-servo · 2017-05-20T02:36:59Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: jdm
Pushing f054911 to master...

highfive assigned cbrewster May 14, 2017

highfive added the S-awaiting-review label May 14, 2017

gterzian mentioned this pull request May 15, 2017

Implement "update the image data" #16238

Merged

1 of 5 tasks complete

jdm requested changes May 15, 2017

View changes

jdm assigned jdm and unassigned cbrewster May 15, 2017

jdm added S-needs-code-changes and removed S-awaiting-review labels May 15, 2017

gterzian force-pushed the gterzian:use_microtask_to_await_stable_state branch from 242bf4d to 0e6bcee May 16, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels May 16, 2017

gterzian force-pushed the gterzian:use_microtask_to_await_stable_state branch 2 times, most recently from 100f6f1 to 98744a0 May 16, 2017

jdm requested changes May 16, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels May 16, 2017

gterzian force-pushed the gterzian:use_microtask_to_await_stable_state branch from 98744a0 to 3fdf6c8 May 17, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels May 17, 2017

gterzian force-pushed the gterzian:use_microtask_to_await_stable_state branch from 3fdf6c8 to b1c4edb May 17, 2017

jdm changed the title ~~[WIP] Use microtasks to await a stable state.~~ Use microtasks to await a stable state. May 17, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels May 19, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels May 19, 2017

use microtasks to await a stable state

Loading status checks…

a8390ae

gterzian force-pushed the gterzian:use_microtask_to_await_stable_state branch from 765d199 to a8390ae May 19, 2017

highfive added S-awaiting-review and removed S-tests-failed labels May 19, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels May 19, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels May 19, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels May 20, 2017

bors-servo merged commit a8390ae into servo:master May 20, 2017
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 20, 2017

gterzian deleted the gterzian:use_microtask_to_await_stable_state branch Jan 7, 2020

servo / servo

Use microtasks to await a stable state. #16861

Use microtasks to await a stable state. #16861

gterzian commented May 14, 2017 •

edited by larsbergstrom

highfive commented May 14, 2017

highfive commented May 14, 2017

gterzian commented May 15, 2017

This comment has been minimized.

This comment has been minimized.

gterzian commented May 16, 2017

jdm commented May 16, 2017

jdm commented May 16, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian commented May 17, 2017

jdm commented May 17, 2017

bors-servo commented May 17, 2017

bors-servo commented May 19, 2017

jdm commented May 19, 2017

gterzian commented May 19, 2017

jdm commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

jdm commented May 19, 2017

bors-servo commented May 20, 2017

bors-servo commented May 20, 2017

servo / servo

Join GitHub today

Use microtasks to await a stable state. #16861

Use microtasks to await a stable state. #16861

Conversation

gterzian commented May 14, 2017 • edited by larsbergstrom

highfive commented May 14, 2017

highfive commented May 14, 2017

gterzian commented May 15, 2017

This comment has been minimized.

jdm May 15, 2017

This comment has been minimized.

jdm May 15, 2017

gterzian commented May 16, 2017

jdm commented May 16, 2017

jdm commented May 16, 2017

This comment has been minimized.

jdm May 16, 2017

This comment has been minimized.

jdm May 16, 2017

This comment has been minimized.

jdm May 16, 2017

gterzian commented May 17, 2017

jdm commented May 17, 2017

bors-servo commented May 17, 2017

bors-servo commented May 19, 2017

jdm commented May 19, 2017

gterzian commented May 19, 2017

jdm commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

bors-servo commented May 19, 2017

jdm commented May 19, 2017

bors-servo commented May 20, 2017

bors-servo commented May 20, 2017

gterzian commented May 14, 2017 •

edited by larsbergstrom