Add task source name #21583

AgustinCB · 2018-09-02T20:27:01Z

Refactor CommonScriptMsg::Task to include TaskSourceName.

Sorry for the delay, between doing this after work and the time I spent trying to ramp up on the project, it took me a bit longer than I expected.

Test still don't pass in local, but they fail the same way on master, so I guess it's ok.

I may have forgotten something, I refactored mostly the stuff that the compiler complained about. Please let me know if I missed anything.

I tried to dump my thought process on the commit messages, so feel free to go commit by commit to understand context.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #21527
There are tests for these changes OR
These changes do not require tests because it's mostly refactor, no new behavior added.

This change is

highfive · 2018-09-02T20:27:07Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @avadacatavra (or someone else) soon.

highfive · 2018-09-02T20:27:08Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/task_source/file_reading.rs, components/script/dom/document.rs, components/script/task_source/remote_event.rs, components/script/task_source/user_interaction.rs, components/script/dom/vrdisplay.rs and 13 more
@KiChjang: components/script/task_source/file_reading.rs, components/script/dom/document.rs, components/script/task_source/remote_event.rs, components/script/task_source/user_interaction.rs, components/script/dom/vrdisplay.rs and 13 more

highfive · 2018-09-02T20:27:09Z

Warning

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

AgustinCB · 2018-09-02T20:28:06Z

components/script/task_queue.rs

            let mut throttled_tasks = self.throttled.borrow_mut();
            throttled_tasks
-                .entry(task_source)
+                .entry(task_source.clone())


I'm not sure this is a good idea.

I thought about refactoring this to use references instead of owned objects, but I didn't felt brave enough and went the easy way.

Any suggestion on what would be better here?

Besides the larger refactoring you suggest, one could also consider using HashMap.get(&task_source_name), however that would come at the cost of more manual logic involved with checking the returned option and inserting an empty Vec if it is None and so on...

Perhaps the convenience of the entry API is worth a (probably not very expensive)clone?

Yeah, that was my intuition here (i.e. the entry call was worth a clone in an small enum). Thank you for the detailed explanation!

AgustinCB · 2018-09-02T20:30:11Z

I wasn't sure about how to (or if I should) add tests for this. If you think is appropriate, could give you give me a quick explanation on how to do it? Thanks!

gterzian

Great work! Glad to see someone picking up this code so quickly! Just a few changes requested, and after that @jdm might want to do a final review(and the commits probably can be squashed into one after)...

gterzian · 2018-09-03T06:34:45Z

components/script/task_queue.rs

            let mut throttled_tasks = self.throttled.borrow_mut();
            throttled_tasks
-                .entry(task_source)
+                .entry(task_source.clone())


Besides the larger refactoring you suggest, one could also consider using HashMap.get(&task_source_name), however that would come at the cost of more manual logic involved with checking the returned option and inserting an empty Vec if it is None and so on...

Perhaps the convenience of the entry API is worth a (probably not very expensive)clone?

gterzian · 2018-09-03T06:34:45Z

components/script/dom/dedicatedworkerglobalscope.rs

+            _ => return None,
+        }
+    }
+


nit: If you insert the new code(in this case task_source_name) at the same level as the old code(in this case task_category), it makes the diff easier to understand for the reviewer(the diff will also show more clearly the difference between the old and new code, since the entire block wasn't changed in fact, see the diff for the other implementations).

In this case, since all other implementations have task_source_name as the first method, as does the trait signature, there is also a benefit in terms of consistency at keeping it the first(top-most) method for this implementation as well.

gterzian · 2018-09-03T06:34:45Z

components/script/dom/dedicatedworkerglobalscope.rs

@@ -447,7 +453,12 @@ impl DedicatedWorkerGlobalScope {
            }
        }));
        // TODO: Should use the DOM manipulation task source.


nit: looks like you can remove this TODO

gterzian · 2018-09-03T06:34:45Z

components/script/dom/dedicatedworkerglobalscope.rs

+            WorkerEvent,
+            task,
+            Some(pipeline_id),
+            TaskSourceName::DOMManipulation,


Could you please add a TODO, and file a corresponding issue, pointing out that we should be using a new unshipped-port-message-queue task source that should be used here?

See https://html.spec.whatwg.org/multipage/#unshipped-port-message-queue (I got to this following the spec link just above the fn PostMessage and looking for the task source that would correspond to this specific tasks, it's often mentioned somewhere else than where the task itself is mentioned, often at the bottom of the relevant section...)

gterzian · 2018-09-03T06:34:45Z

components/script/dom/document.rs

+            ScriptThreadEventCategory::EnterFullscreen,
+            handler,
+            pipeline_id,
+            TaskSourceName::DOMManipulation,


Please add a NOTE here that the spec mentions running these steps "in parallel", not necessarily as a queued task. For now using the DOMManipulation seems OK however. See Step 7 of https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen

I've filed a bit more of an explanation at #21600

gterzian · 2018-09-03T06:34:45Z

components/script/dom/document.rs

+            ScriptThreadEventCategory::ExitFullscreen,
+            handler,
+            pipeline_id,
+            TaskSourceName::DOMManipulation,


Same here, see Step 8 of https://fullscreen.spec.whatwg.org/#exit-fullscreen

gterzian · 2018-09-03T06:34:45Z

components/script/dom/vrdisplay.rs

+                    WebVREvent,
+                    task,
+                    Some(pipeline_id),
+                    TaskSourceName::DOMManipulation,


Please add a NOTE asking whether long-term we want to be using the DOMManipulation task source here. The WebVR spec doesn't seem to point to s specific task source.

gterzian · 2018-09-03T06:34:45Z

components/script/dom/websocket.rs

+                    WebSocketEvent,
+                    task,
+                    Some(pipeline_id),
+                    TaskSourceName::Networking,


Please add a TODO, and file an issue, noting that we should be using a dedicated websocket-task-source here, see https://html.spec.whatwg.org/multipage/web-sockets.html#websocket-task-source (I got to this by first looking up this BufferedAmountTask, following the spec link, and then scrolling down until I found the words "The task source for all tasks queued in this section is the WebSocket task source.")

gterzian · 2018-09-03T06:34:45Z

components/script/dom/worklet.rs

+            ScriptThreadEventCategory::WorkletEvent,
+            Box::new(task),
+            None,
+            TaskSourceName::DOMManipulation,


Please add a NOTE that it's unclear from the Spec which task source should be used here, see https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

gterzian · 2018-09-03T06:37:09Z

The CI error looks unrelated https://travis-ci.org/servo/servo/jobs/423696837#L2135, was it building fine on your machine?

By the way, the relevant test that you might want to run on your machine to see if it passes, would be

./mach test-wpt /_mozilla/mozilla/task_queue_throttling.any.worker.html
and ./mach test-wpt /_mozilla/mozilla/task_queue_throttling.any.html

I would tend to think that no new test would be required for this change, as the above, and many other tests in the wpt suite, should cover it.

You probably don't want to run the entire suite on your machine, as there will be lots of spurious timeouts, those can be run on the server from here.

AgustinCB · 2018-09-03T12:37:02Z

Yes, it builds correctly on local.

And both test succeed on local too!

gterzian · 2018-09-03T13:15:11Z

components/script/task_queue.rs

@@ -82,7 +88,8 @@ impl<T: QueuedTaskConversion> TaskQueue<T> {
        for msg in to_be_throttled {
            // Categorize tasks per task queue.
            let (worker, category, boxed, pipeline_id, task_source) = match msg.into_queued_task() {
-                Some((worker, category, boxed, pipeline_id, task_source)) => (worker, category, boxed, pipeline_id, task_source),
+                Some(queued_task) =>
+                    queued_task,


that's a nice one, there was indeed no need to unwrap the queued_task right inside the match...

gterzian · 2018-09-03T13:24:18Z

Ok, thanks for giving us context with your commits, I'd say one could have squashed them into two commits:

Task-source: add the TaskSourceName to CommonScriptMsg::Task(consisting of the first two commits).
Task-queue: replace task_category with task_source_name in QueuedTaskConversion(consisting of only the changes to QueuedTaskConversion and the task-queue that relate to replacing the task_category method with task_source)

However since the third commit also contains some smaller changes like 5c432f5#diff-1879ac6bd5d567e2aa43529d33474677L274 that are corrections of the previous commit, I think you might have a bit of a hard time with git to dis-entangle those changes using the edit option of git rebase and breaking those changes into new smaller commits, later re-squashing them as appropriate in those two mentioned above(same goes for the last two commits, which I think each have parts that would relate to either 1 or 2).

To make it easier, perhaps you could squash all into one commit entitled add the TaskSourceName to CommonScriptMsg::Task and update QueuedTaskConversion and the TaskQueue to use it?

gterzian

One additional nit

gterzian · 2018-09-03T13:37:55Z

components/script/task_queue.rs

-                Some((worker, category, boxed, pipeline_id)) => (worker, category, boxed, pipeline_id),
+            let (worker, category, boxed, pipeline_id, task_source) = match msg.into_queued_task() {
+                Some(queued_task) =>
+                    queued_task,


nit: I think this one would belong on the same line right after the =>, or has rustfmt formatted it that way?
(not saying you should run rustfmt if you haven't, since that would likely introduce other changes unrelated to the present commits)

gterzian

One more set of nits :)

gterzian · 2018-09-03T14:30:08Z

components/script/task_source/user_interaction.rs

@@ -40,7 +40,8 @@ impl TaskSource for UserInteractionTaskSource {
        let msg = MainThreadScriptMsg::Common(CommonScriptMsg::Task(
            ScriptThreadEventCategory::InputEvent,
            Box::new(canceller.wrap_task(task)),
-            Some(self.1)
+            Some(self.1),
+            TaskSourceName::UserInteraction,


For all the changes inside implementations of TaskSource, like this one, I think you can use the associated const directly, doing something like UserInteractionTaskSource::NAME

See https://doc.rust-lang.org/reference/items/associated-items.html#associated-constants

gterzian · 2018-09-03T15:24:15Z

Thanks! Please squash, and we can then try a full test run on the server...

AgustinCB · 2018-09-03T15:25:55Z

Done!

gterzian · 2018-09-03T16:05:29Z

@bors-servo try

bors-servo · 2018-09-03T16:05:34Z

⌛ Trying commit 222a537 with merge 4b76b2a...


        Auto merge of #21583 - AgustinCB:add-task-source-name, r=<try>

Add task source name Refactor `CommonScriptMsg::Task` to include `TaskSourceName`. Sorry for the delay, between doing this after work and the time I spent trying to ramp up on the project, it took me a bit longer than I expected. Test still don't pass in local, but they fail the same way on master, so I guess it's ok. I may have forgotten something, I refactored mostly the stuff that the compiler complained about. Please let me know if I missed anything. I tried to dump my thought process on the commit messages, so feel free to go commit by commit to understand context. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21527 - [ ] There are tests for these changes OR - [x] These changes do not require tests because it's mostly refactor, no new behavior added.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21583)

bors-servo · 2018-09-03T16:31:41Z

💔 Test failed - linux-rel-wpt

gterzian · 2018-09-03T17:11:09Z


{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_webgl/conformance/context/context-release-upon-reload.html", "line": 199837, "action": "test_result", "expected": "OK"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_webgl/conformance2/rendering/draw-buffers.html", "line": 199851, "action": "test_result", "expected": "OK"}

#21595
#21596

Only a few intermittent failure(which you can find by going to http://build.servo.org/grid and looking at the commit corresponding to what is announced at #21583 (comment))

@jdm r?

AgustinCB · 2018-09-03T18:57:58Z

Is there something I can do to make those tests pass?

gterzian · 2018-09-04T05:41:58Z

I don't think so, those are tests that will fail on occasion and that aren't related to the current changes(I think).

We can wait for @jdm to give the greenlight, or another round of review if necessary...

gterzian

One small change...

gterzian · 2018-09-04T05:45:11Z

components/script/task_source/networking.rs

@@ -46,6 +47,7 @@ impl NetworkingTaskSource {
            ScriptThreadEventCategory::NetworkEvent,
            Box::new(task),
            Some(self.1),
+            TaskSourceName::Networking,


Looks like we missed one case of using the associated const here.

gterzian

One more thing that slipped my attention earlier...

gterzian · 2018-09-04T13:09:14Z

components/script/dom/vrdisplay.rs

@@ -517,7 +518,14 @@ impl VRDisplay {
                let task = Box::new(task!(handle_vrdisplay_raf: move || {
                    this.root().handle_raf(&sender);
                }));
-                js_sender.send(CommonScriptMsg::Task(WebVREvent, task, Some(pipeline_id))).unwrap();
+                // NOTE: WebVR spec doesn't specify what task source we should use. Is
+                // dom-manipulation a good choise long term?


nit: typo in the comment with choise...


        Add the TaskSourceName to CommonScriptMsg::Task

Update QueuedTaskConversion and the TaskQueue to use it

jdm · 2018-09-04T17:38:07Z

Great work @AgustinCB, and thanks for reviewing the changes, @gterzian! I've looked them over and everything looks like what I expected :)

@bors-servo r+

bors-servo · 2018-09-04T17:38:08Z

📌 Commit e286fdc has been approved by jdm

bors-servo · 2018-09-04T18:17:35Z

⌛ Testing commit e286fdc with merge 162826d...


        Auto merge of #21583 - AgustinCB:add-task-source-name, r=jdm

Add task source name Refactor `CommonScriptMsg::Task` to include `TaskSourceName`. Sorry for the delay, between doing this after work and the time I spent trying to ramp up on the project, it took me a bit longer than I expected. Test still don't pass in local, but they fail the same way on master, so I guess it's ok. I may have forgotten something, I refactored mostly the stuff that the compiler complained about. Please let me know if I missed anything. I tried to dump my thought process on the commit messages, so feel free to go commit by commit to understand context. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #21527 - [ ] There are tests for these changes OR - [x] These changes do not require tests because it's mostly refactor, no new behavior added.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21583)

bors-servo · 2018-09-04T19:22:31Z

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 162826d to master...

highfive assigned avadacatavra Sep 2, 2018

highfive added the S-awaiting-review label Sep 2, 2018

AgustinCB reviewed Sep 2, 2018

View changes

gterzian suggested changes Sep 3, 2018

View changes

gterzian reviewed Sep 3, 2018

View changes

gterzian mentioned this pull request Sep 3, 2018

"No space left on device" on Travis CI #21592

Closed

gterzian suggested changes Sep 3, 2018

View changes

AgustinCB force-pushed the AgustinCB:add-task-source-name branch from a8c65e0 to d65ba4f Sep 3, 2018

gterzian suggested changes Sep 3, 2018

View changes

AgustinCB force-pushed the AgustinCB:add-task-source-name branch from 9248028 to 222a537 Sep 3, 2018

highfive added the S-tests-failed label Sep 3, 2018

gterzian suggested changes Sep 4, 2018

View changes

AgustinCB force-pushed the AgustinCB:add-task-source-name branch from 222a537 to eeaec31 Sep 4, 2018

highfive removed the S-tests-failed label Sep 4, 2018

gterzian suggested changes Sep 4, 2018

View changes

Add the TaskSourceName to CommonScriptMsg::Task

Loading status checks…

e286fdc

Update QueuedTaskConversion and the TaskQueue to use it

AgustinCB force-pushed the AgustinCB:add-task-source-name branch from eeaec31 to e286fdc Sep 4, 2018

highfive assigned jdm and unassigned avadacatavra Sep 4, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 4, 2018

bors-servo merged commit e286fdc into servo:master Sep 4, 2018
1 of 3 checks passed

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 4, 2018

bors-servo mentioned this pull request Sep 4, 2018

Replace mpsc with crossbeam-channel #21325

Merged

gterzian mentioned this pull request Sep 5, 2018

Add unshipped port message queue task #21609

Closed

4 of 5 tasks complete

servo / servo

Join GitHub today

Add task source name #21583

Add task source name #21583

Conversation

AgustinCB commented Sep 2, 2018 • edited by SimonSapin

highfive commented Sep 2, 2018

highfive commented Sep 2, 2018

highfive commented Sep 2, 2018

This comment has been minimized.

This comment has been minimized.

gterzian Sep 3, 2018 • edited

This comment has been minimized.

AgustinCB commented Sep 2, 2018

gterzian left a comment • edited

This comment has been minimized.

gterzian Sep 3, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian Sep 3, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian Sep 3, 2018 • edited

This comment has been minimized.

This comment has been minimized.

gterzian commented Sep 3, 2018 • edited

AgustinCB commented Sep 3, 2018

This comment has been minimized.

gterzian commented Sep 3, 2018 • edited

gterzian left a comment

This comment has been minimized.

gterzian Sep 3, 2018 • edited

gterzian left a comment

This comment has been minimized.

gterzian Sep 3, 2018 • edited

gterzian commented Sep 3, 2018 • edited

AgustinCB commented Sep 3, 2018

gterzian commented Sep 3, 2018

bors-servo commented Sep 3, 2018

bors-servo commented Sep 3, 2018

gterzian commented Sep 3, 2018 • edited

AgustinCB commented Sep 3, 2018

gterzian commented Sep 4, 2018 • edited

gterzian left a comment

This comment has been minimized.

gterzian Sep 4, 2018 • edited

gterzian left a comment

This comment has been minimized.

gterzian Sep 4, 2018 • edited

jdm commented Sep 4, 2018

bors-servo commented Sep 4, 2018

bors-servo commented Sep 4, 2018

bors-servo commented Sep 4, 2018

AgustinCB commented Sep 2, 2018 •

edited by SimonSapin

gterzian Sep 3, 2018 •

edited

gterzian left a comment •

edited

gterzian Sep 3, 2018 •

edited

gterzian Sep 3, 2018 •

edited

gterzian Sep 3, 2018 •

edited

gterzian commented Sep 3, 2018 •

edited

gterzian commented Sep 3, 2018 •

edited

gterzian Sep 3, 2018 •

edited

gterzian Sep 3, 2018 •

edited

gterzian commented Sep 3, 2018 •

edited

gterzian commented Sep 3, 2018 •

edited

gterzian commented Sep 4, 2018 •

edited

gterzian Sep 4, 2018 •

edited

gterzian Sep 4, 2018 •

edited