Remove util::ipc module (use ipc_channel instead; fixes #12312) #12558

djc · 2016-07-22T14:56:45Z

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

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

This change is

highfive · 2016-07-22T14:56:49Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/pipeline.rs
@KiChjang: components/script_layout_interface/lib.rs, components/script_layout_interface/Cargo.toml, components/script_layout_interface/message.rs, components/script_traits/Cargo.toml, components/script_traits/Cargo.toml, components/script_traits/lib.rs, components/script_traits/lib.rs

highfive · 2016-07-22T14:56:50Z

Warning

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

djc · 2016-07-22T14:58:48Z

Rebasing, please stand by...


        Remove util::ipc module (use ipc_channel instead)

pcwalton · 2016-07-22T17:18:51Z

Cool! Happy to see one more out-of-place module go away. :)

asajeffrey · 2016-07-25T19:35:46Z

Reviewed 4 of 14 files at r1, 11 of 11 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

asajeffrey · 2016-07-25T19:36:27Z

Yay, one less dependency on util! @bors-servo r+

bors-servo · 2016-07-25T19:36:28Z

📌 Commit 222feb4 has been approved by asajeffrey

bors-servo · 2016-07-25T19:36:30Z

⌛ Testing commit 222feb4 with merge b63d440...


        Auto merge of #12558 - djc:rm-util-ipc, r=asajeffrey

Remove util::ipc module (use ipc_channel instead; fixes #12312)  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12312 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring only   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12558)

bors-servo · 2016-07-25T20:37:12Z

💔 Test failed - linux-rel

KiChjang · 2016-07-25T20:41:47Z

From ./mach test-wpt:

Ran 6788 tests finished in 866.0 seconds.
  • 6786 ran as expected. 1998 tests skipped.
  • 2 tests timed out unexpectedly
  • 1 tests had unexpected subtest results

Tests with unexpected results:
  ▶ TIMEOUT [expected OK] /_mozilla/mozilla/mozbrowser/mozbrowser_loadevents.html
  │ 
  └ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".

  ▶ Unexpected subtest result in /_mozilla/mozilla/mozbrowser/mozbrowser_loadevents.html:
  │ TIMEOUT [expected PASS] mozbrowserloadstart, mozbrowserconnected and mozbrowserloadend are dispatched
  └   → Test timed out

  ▶ TIMEOUT [expected OK] /_mozilla/mozilla/mozbrowser/mozbrowsersecuritychange_event.html
  │ 
  └ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".

From ./mach test-css:

Ran 9977 tests finished in 1143.0 seconds.
  • 9971 ran as expected. 132 tests skipped.
  • 6 tests timed out unexpectedly

Tests with unexpected results:
  ▶ TIMEOUT [expected FAIL] /css-text-3_dev/html/line-break-normal-023.htm
  │ 
  ��� Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".

  ▶ TIMEOUT [expected FAIL] /css-text-3_dev/html/line-break-strict-016.htm
  │ 
  └ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".

  ▶ TIMEOUT [expected FAIL] /css-text-3_dev/html/line-break-strict-017.htm
  │ 
  └ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".

  ▶ TIMEOUT [expected FAIL] /css21_dev/html4/abspos-replaced-width-margin-000.htm
  │ 
  │ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 22, message: "Invalid argument" } }
  │ ERROR:servo: assertion failed: !self.Document().needs_reflow() || self.window_size.get().is_none() ||
  │     self.suppress_reflow.get()
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  └ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError

  ▶ TIMEOUT [expected FAIL] /css21_dev/html4/border-conflict-element-001d.htm
  │ 
  │ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".
  │ ERROR:servo: called `Option::unwrap()` on a `None` value
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  └ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError

  ▶ TIMEOUT [expected FAIL] /css21_dev/html4/border-conflict-element-001e.htm
  │ 
  │ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 32, message: "Broken pipe" } }
  │ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".
  │ ERROR:servo: called `Option::unwrap()` on a `None` value
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  │ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
  └ ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError

Ms2ger · 2016-07-26T08:45:39Z

Let's see if this was bad luck or something more serious...

@bors-servo retry

bors-servo · 2016-07-26T08:45:41Z

⌛ Testing commit 222feb4 with merge e8ed72f...


        Auto merge of #12558 - djc:rm-util-ipc, r=asajeffrey

Remove util::ipc module (use ipc_channel instead; fixes #12312)  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12312 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring only   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12558)

bors-servo · 2016-07-26T09:47:00Z

💔 Test failed - linux-rel

wafflespeanut · 2016-07-26T10:01:54Z

Getting the same error again.

Ms2ger · 2016-07-27T06:33:37Z

Ugh. @antrik, we're hitting timeouts due to switching from mpsc channels to ipc ones (for in-process communication). Is that something you'd be interested in looking into?

antrik · 2016-07-29T00:15:17Z

It turns out the timeouts (at least the test-css ones) are not due to some kind of deadlock, but rather the tests indeed become really slow with this change.

This mostly seems to go back to #10230 , i.e. the fact that font data presently is copied rather than shared when sending over IPC channels. There is a lot of that going on here; and while I made the actual send over the socket in ipc-channel pretty fast, the serialisation done by serde still takes a lot of time.

This doesn't happen with webrender, presumably because that uses a separate "raw" bytes_channel for payload -- but maybe also the font handling is done differently there? Regardless of the broken font cache that needs to be fixed, sending so many fonts to the paint thread doesn't seem quite right in the first place. (It's much more font data than what is sent to the layout threads -- which suggests the same fonts are being sent to the paint thread over and over or something like that...)

asajeffrey · 2016-07-29T16:46:38Z

components/gfx/paint_thread.rs

@@ -373,7 +375,7 @@ impl<C> PaintThread<C> where C: PaintListener + Send + 'static {
    pub fn create(id: PipelineId,
                  url: Url,
                  chrome_to_paint_chan: Sender<ChromeToPaintMsg>,
-                  layout_to_paint_port: Receiver<LayoutToPaintMsg>,
+                  layout_to_paint_port: IpcReceiver<LayoutToPaintMsg>,


Ah, is this replacing a channel by an ipc channel? Is this the source of slowdown?

Oh, definitely don't do that. That will be very slow!

This is replacing MPSC channels with IPC channels for the single-process case (still the default); it was (obviously) already using IPC channels in the multi-process case.

I don't think we should rely on single-process optimisation if we ever want to make the multi-process case default and/or actually viable. So I don't think this change is bad per se -- it just brings out problems that would have come up anyway if the tests were run with -M...

bors-servo · 2016-07-29T18:53:45Z

☔ The latest upstream changes (presumably #12637) made this pull request unmergeable. Please resolve the merge conflicts.

antrik · 2016-07-29T20:48:28Z

components/gfx/paint_thread.rs

@@ -392,7 +394,8 @@ impl<C> PaintThread<C> where C: PaintListener + Send + 'static {
            let mut paint_thread = PaintThread {
                id: id,
                _url: url,
-                layout_to_paint_port: layout_to_paint_port,
+                layout_to_paint_port:
+                    ROUTER.route_ipc_receiver_to_new_mpsc_receiver(layout_to_paint_port),


It seems a little arbitrary, and possibly confusing, to do the wrapping here: I think it might be clearer to do it right at the place where layout_to_paint_port is originally created -- thus keeping it closer to what the original code did...

djc · 2016-07-31T08:22:22Z

Just to be explicit, I don't have a clue right now how to make progress with this, so I'll either need a bit more (concrete) guidance or maybe someone else can pick it up.

asajeffrey · 2016-08-01T17:24:15Z

@djc can we land a change which just gets rid of util/ipc without making other changes, in particular without replacing MPSC channels by IPC channels?

antrik · 2016-08-01T18:05:33Z

@asajeffrey not really -- unless reimplementing the logic of util/ipc ad hoc, which is surely not the point...

antrik · 2016-08-01T18:13:42Z

@djc just to avoid possible misunderstanding: your patch is fine in itself (aside for the minor nitpick I pointed out maybe); but it cannot be applied for now, because it exposes other problems, which are not entirely easy to fix.

antrik · 2016-08-01T18:15:59Z

@asajeffrey let me elaborate, as there seems to be some confusion here: the whole point of util/ipc is to use MPSC channels when running single-process, and IPC channels when running multi-process. The only way to get rid of it (except by replicating its logic, which I wouldn't consider a win) is by using IPC channels unconditionally. That's precisely what the patch does.

djc · 2016-08-01T18:27:41Z

So I guess that as a precursor, some work is needed to make the way the formerly-MPSC channels need to be used more efficiently, somehow?

antrik · 2016-08-01T18:55:28Z

@djc exactly.

As I said, a major win will be fixing the font cache to work again in the face of ipc-channel (so it doesn't copy the actual font data over the IPC channel every time) -- which is something I have been pondering on and off for quite some time, but will be pursuing more actively now.

Independent of that, it might be worth investigating why the layout thread is sending such a huge amount of font data to the paint thread: checking whether this is intended behaviour (relying on a working cache), or some kind of bug/omission...

Ms2ger · 2016-10-19T08:07:51Z

Rebased in #13831.

highfive assigned asajeffrey Jul 22, 2016

highfive added the S-awaiting-review label Jul 22, 2016

Remove util::ipc module (use ipc_channel instead)

Loading status checks…

222feb4

djc force-pushed the djc:rm-util-ipc branch from c1f5e5e to 222feb4 Jul 22, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 25, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 25, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 26, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 26, 2016

asajeffrey reviewed Jul 29, 2016
View changes

highfive added the S-needs-rebase label Jul 29, 2016

antrik reviewed Jul 29, 2016
View changes

jdm added the C-looking for assistance label Jul 31, 2016

Ms2ger closed this Oct 19, 2016

servo / servo

Remove util::ipc module (use ipc_channel instead; fixes #12312) #12558

Remove util::ipc module (use ipc_channel instead; fixes #12312) #12558

djc commented Jul 22, 2016 •

edited by larsbergstrom

highfive commented Jul 22, 2016

highfive commented Jul 22, 2016

djc commented Jul 22, 2016

pcwalton commented Jul 22, 2016

asajeffrey commented Jul 25, 2016

asajeffrey commented Jul 25, 2016

bors-servo commented Jul 25, 2016

bors-servo commented Jul 25, 2016

bors-servo commented Jul 25, 2016

KiChjang commented Jul 25, 2016

Ms2ger commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

wafflespeanut commented Jul 26, 2016

Ms2ger commented Jul 27, 2016

antrik commented Jul 29, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jul 29, 2016

This comment has been minimized.

djc commented Jul 31, 2016

asajeffrey commented Aug 1, 2016

antrik commented Aug 1, 2016

antrik commented Aug 1, 2016

antrik commented Aug 1, 2016

djc commented Aug 1, 2016

antrik commented Aug 1, 2016

Ms2ger commented Oct 19, 2016

servo / servo

Join GitHub today

Remove util::ipc module (use ipc_channel instead; fixes #12312) #12558

Remove util::ipc module (use ipc_channel instead; fixes #12312) #12558

Conversation

djc commented Jul 22, 2016 • edited by larsbergstrom

highfive commented Jul 22, 2016

highfive commented Jul 22, 2016

djc commented Jul 22, 2016

pcwalton commented Jul 22, 2016

asajeffrey commented Jul 25, 2016

asajeffrey commented Jul 25, 2016

bors-servo commented Jul 25, 2016

bors-servo commented Jul 25, 2016

bors-servo commented Jul 25, 2016

KiChjang commented Jul 25, 2016

Ms2ger commented Jul 26, 2016

bors-servo commented Jul 26, 2016

bors-servo commented Jul 26, 2016

wafflespeanut commented Jul 26, 2016

Ms2ger commented Jul 27, 2016

antrik commented Jul 29, 2016

This comment has been minimized.

asajeffrey Jul 29, 2016

This comment has been minimized.

pcwalton Jul 29, 2016

This comment has been minimized.

antrik Jul 29, 2016 • edited

bors-servo commented Jul 29, 2016

This comment has been minimized.

antrik Jul 29, 2016

djc commented Jul 31, 2016

asajeffrey commented Aug 1, 2016

antrik commented Aug 1, 2016

antrik commented Aug 1, 2016

antrik commented Aug 1, 2016

djc commented Aug 1, 2016

antrik commented Aug 1, 2016

Ms2ger commented Oct 19, 2016

djc commented Jul 22, 2016 •

edited by larsbergstrom

antrik Jul 29, 2016 •

edited