Restructure content process shutdown ack with threaded sender #23972

gterzian · 2019-08-15T07:10:10Z

Recycling some work from #23909

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

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

This change is

highfive · 2019-08-15T07:10:15Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmliframeelement.rs, components/constellation/pipeline.rs, components/script/dom/windowproxy.rs, components/script/script_thread.rs
@cbrewster: components/constellation/pipeline.rs
@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/windowproxy.rs, components/script/script_thread.rs, components/script_traits/lib.rs

highfive · 2019-08-15T07:10:18Z

Warning

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

gterzian · 2019-08-15T07:12:42Z

This is another recycled commit from the big ipc experiment over at #23909

In this case, it restructures the "wait for content to shutdown" pattern, in the following way:

Since it's only used in multiprocess mode, and the "waiting" is done from the "main thread" in the content process, it seems like a threaded channel, created at the point of use, will suffice.
I remove the "wait on layout" part, since the script-thread will itself wait on layout to shutdown each time it shuts down a layout thread, so it seems to me the main-thread only needs to wait on script?
Since dropping the only sender in existence for that channel will already wake-up the receiver, it seems unnecessary to send an explicit message. Instead, we just store the sender on the script-thread, and it will drop along with it when it exits and notify the main thread(of the content-process).

@asajeffrey r?

jdm · 2019-08-15T16:24:28Z

r? @asajeffrey

asajeffrey

I'm a bit confused by these changes. Replacing an IPC channel by a crossbeam channel IO can understand, since the channel never leaves the process. But AFAICT the channel is being used as a latch, we're never actually sending a message on it?

asajeffrey · 2019-08-15T16:30:02Z

components/constellation/pipeline.rs

+            let signal = content_process_shutdown_port.recv();
+            // When the script-thread drops the sender,
+            // we expect to receive an Err(_).
+            assert!(signal.is_err());


Perhaps an error! log rather than a panic?

Oh yeah, forgot this is in the constellation thread...

It's not in the constellation thread actually, but I guess an error is better.

asajeffrey · 2019-08-15T16:44:17Z

components/constellation/pipeline.rs

+            let signal = content_process_shutdown_port.recv();
+            // When the script-thread drops the sender,
+            // we expect to receive an Err(_).
+            assert!(signal.is_err());


I'm confused about who sends on this channel, since the only uses of send(()) appear to have been deleted. Are we using a channel just for its Drop implementation?

Yes, previously there was an explicit sending of a message when the script-thread exits, which I can bring back if you think that's clearer. Or we can effectively wait until the channel drops, and assert it is indeed the Err that would be received in such a case.

I think we can count on the channel not sending any spurious errors.

Actually, I think you're right it's better to send an explicit message, since the drop could also be the result of an unexpected shutdown of the script-thread, while a message will almost always mean there was a graceful shutdown...

asajeffrey · 2019-08-15T16:44:53Z

components/script/script_thread.rs

@@ -744,6 +744,7 @@ impl ScriptThreadFactory for ScriptThread {
                thread_state::initialize(ThreadState::SCRIPT);
                PipelineNamespace::install(state.pipeline_namespace_id);
                TopLevelBrowsingContextId::install(state.top_level_browsing_context_id);
+


Don't think we need a new blank line :)

gterzian · 2019-08-15T22:17:24Z

Ok, comments addressed @asajeffrey

asajeffrey

OK, a question, but otherwise LGTM.

asajeffrey · 2019-08-16T14:44:23Z

components/constellation/pipeline.rs

@@ -545,6 +530,7 @@ impl UnprivilegedPipelineContent {
            self.script_chan.clone(),
            self.load_data.url.clone(),
        );
+        let (content_process_shutdown_chan, content_process_shutdown_port) = unbounded();


Should this be unbounded or a one-shot channel?

I'm not aware of crossbeam having a dedicated one-shot variant.

I meant bounded(1).

for what reason?

Possible space efficiency, but it probably doesn't matter that much.

I see what you mean, counterintuitively, the unbounded variant appears to require less bookkeeping than the bounded variant.

asajeffrey · 2019-08-17T01:15:36Z

@bors-servo r+

bors-servo · 2019-08-17T01:15:37Z

📌 Commit 4cd2730 has been approved by asajeffrey

bors-servo · 2019-08-17T03:00:25Z

⌛ Testing commit 4cd2730 with merge e0267e8...

… r=asajeffrey Restructure content process shutdown ack with threaded sender Recycling some work from #23909  ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (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/23972)

bors-servo · 2019-08-17T05:37:32Z

💔 Test failed - status-taskcluster

…t layout

gterzian · 2019-08-17T14:04:00Z

@asajeffrey Ok that was layout_2020 that required updating, done...

asajeffrey · 2019-08-17T14:42:16Z

@bors-servo r+

bors-servo · 2019-08-17T14:42:16Z

📌 Commit 8482d2e has been approved by asajeffrey

bors-servo · 2019-08-17T16:24:57Z

⌛ Testing commit 8482d2e with merge 0ff5dc5...

… r=asajeffrey Restructure content process shutdown ack with threaded sender Recycling some work from #23909  ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (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/23972)

bors-servo · 2019-08-17T19:19:53Z

💔 Test failed - status-taskcluster

gterzian · 2019-08-17T19:33:26Z

@bors-servo retry

seems like a taskcluster error: TASK FAILURE during artifact upload: file-missing-on-worker: Could not read file '/Users/worker/tasks/task_1566064170/repo/intermittents.log'

bors-servo · 2019-08-17T21:33:01Z

💣 Failed to start rebuilding: Unknown error

bors-servo · 2019-08-17T21:33:05Z

⌛ Testing commit 8482d2e with merge a8ae0e5...

… r=asajeffrey Restructure content process shutdown ack with threaded sender Recycling some work from #23909  ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (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/23972)

bors-servo · 2019-08-17T22:51:09Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: asajeffrey
Pushing a8ae0e5 to master...

highfive assigned nox Aug 15, 2019

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 15, 2019

gterzian requested a review from asajeffrey August 15, 2019 07:10

highfive assigned asajeffrey and unassigned nox Aug 15, 2019

asajeffrey reviewed Aug 15, 2019

View reviewed changes

gterzian force-pushed the cleanup_content_process_shutdown_ack branch 2 times, most recently from afc6480 to 4cd2730 Compare August 15, 2019 19:47

gterzian requested a review from asajeffrey August 16, 2019 11:10

asajeffrey reviewed Aug 16, 2019

View reviewed changes

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 17, 2019

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 17, 2019

restructure content process shutdown ack with threaded sender, withou…

8482d2e

…t layout

gterzian force-pushed the cleanup_content_process_shutdown_ack branch from 4cd2730 to 8482d2e Compare August 17, 2019 14:03

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 17, 2019

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 17, 2019

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 17, 2019

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 17, 2019

bors-servo merged commit 8482d2e into servo:master Aug 17, 2019

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 17, 2019

bors-servo mentioned this pull request Aug 17, 2019

[WIP]Introduce a shared ipc router and a ipc handle #23909

Closed

5 tasks

gterzian deleted the cleanup_content_process_shutdown_ack branch August 18, 2019 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure content process shutdown ack with threaded sender #23972

Restructure content process shutdown ack with threaded sender #23972

gterzian commented Aug 15, 2019 •

edited by SimonSapin

highfive commented Aug 15, 2019

highfive commented Aug 15, 2019

gterzian commented Aug 15, 2019 •

edited

jdm commented Aug 15, 2019

asajeffrey left a comment

asajeffrey Aug 15, 2019

gterzian Aug 15, 2019

gterzian Aug 15, 2019

asajeffrey Aug 15, 2019

gterzian Aug 15, 2019 •

edited

gterzian Aug 15, 2019 •

edited

asajeffrey Aug 15, 2019

gterzian commented Aug 15, 2019

asajeffrey left a comment

asajeffrey Aug 16, 2019

gterzian Aug 16, 2019

asajeffrey Aug 16, 2019

gterzian Aug 16, 2019

asajeffrey Aug 17, 2019

gterzian Aug 17, 2019

asajeffrey commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 17, 2019

asajeffrey commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

Navigation Menu

Restructure content process shutdown ack with threaded sender #23972

Restructure content process shutdown ack with threaded sender #23972

Conversation

gterzian commented Aug 15, 2019 • edited by SimonSapin

highfive commented Aug 15, 2019

highfive commented Aug 15, 2019

gterzian commented Aug 15, 2019 • edited

jdm commented Aug 15, 2019

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian Aug 15, 2019 • edited

Choose a reason for hiding this comment

gterzian Aug 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gterzian commented Aug 15, 2019

asajeffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asajeffrey commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 17, 2019

asajeffrey commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

bors-servo commented Aug 17, 2019

gterzian commented Aug 15, 2019 •

edited by SimonSapin

gterzian commented Aug 15, 2019 •

edited

gterzian Aug 15, 2019 •

edited

gterzian Aug 15, 2019 •

edited