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 upscript: Make the resource task communication use IPC channels. #6586
Conversation
hoppipolla-critic-bot
commented
Jul 10, 2015
|
Critic review: https://critic.hoppipolla.co.uk/r/5505 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
|
I'm not so happy about the SerializableFoo wrapper structs; can we avoid those somehow? |
|
Only by making our upstream crates take hard dependencies on serde, which I didn't want to do since serde is pretty immature still. |
|
Cargo features? |
|
Everything has gone upstream now except the serde change. However, we can't take the hyper upgrade yet until the rustup lands, because of an ICE. That rustup is itself blocked on mysterious linker errors. We could also fork hyper and backport the change to get ourselves out of this mess. |
|
|
|
-S-awaiting-review +S-needs-code-changes Reviewed 2 of 20 files at r1, 1 of 10 files at r2, 4 of 17 files at r3, 21 of 29 files at r5, 8 of 8 files at r6. components/devtools_traits/Cargo.toml, line 22 [r6] (raw file): components/net/data_loader.rs, line 33 [r6] (raw file): components/net/http_loader.rs, line 366 [r6] (raw file): components/net/http_loader.rs, line 379 [r6] (raw file): components/net/image_cache_task.rs, line 108 [r6] (raw file): components/net/image_cache_task.rs, line 257 [r6] (raw file): components/net/image_cache_task.rs, line 343 [r6] (raw file): components/net/image_cache_task.rs, line 346 [r6] (raw file): components/net/resource_task.rs, line 282 [r6] (raw file): components/net_traits/lib.rs, line 34 [r6] (raw file): components/script/Cargo.toml, line 68 [r6] (raw file): components/script/dom/htmlscriptelement.rs, line 343 [r6] (raw file): components/script/dom/xmlhttprequest.rs, line 283 [r6] (raw file): components/script/dom/xmlhttprequest.rs, line 798 [r6] (raw file): components/script/network_listener.rs, line 20 [r6] (raw file): components/script/script_task.rs, line 1523 [r6] (raw file): components/util/Cargo.toml, line 25 [r6] (raw file): Comments from the review on Reviewable.io |
|
components/util/Cargo.toml, line 25 [r6] (raw file): Comments from the review on Reviewable.io |
|
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed. components/net/data_loader.rs, line 33 [r6] (raw file): components/net/image_cache_task.rs, line 108 [r6] (raw file): components/script/network_listener.rs, line 20 [r6] (raw file): Comments from the review on Reviewable.io |
|
Addressed the review comments. r? @jdm |
|
-S-awaiting-review +S-needs-code-changes Reviewed 1 of 20 files at r1, 31 of 31 files at r7. components/net/data_loader.rs, line 33 [r6] (raw file): components/servo/Cargo.lock, line 1464 [r7] (raw file): ports/cef/Cargo.lock, line 1116 [r7] (raw file): ports/cef/Cargo.lock, line 1411 [r7] (raw file): ports/gonk/Cargo.lock, line 1300 [r7] (raw file): Comments from the review on Reviewable.io |
|
Review status: 31 of 35 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful. ports/cef/Cargo.lock, line 1116 [r7] (raw file): ports/cef/Cargo.lock, line 1411 [r7] (raw file): Comments from the review on Reviewable.io |
|
|
|
|
|
|
script: Make the resource task communication use IPC channels. This change makes Servo use serialized messages over IPC channels for resource loading. The goal is to make it easier to make Servo multiprocess in the future. This patch does not make Servo multiprocess now; there are many other channels that need to be changed to IPC before that can happen. It does introduce a dependency on https://github.com/serde-rs/serde and https://github.com/pcwalton/ipc-channel for the first time. At the moment, `ipc-channel` uses JSON for serialization. This is because serde does not yet have official support for bincode. When serde gains support for bincode, I'll switch to that. For now, however, the JSON encoding and decoding will constitute a significant performance regression in resource loading. To avoid having to send boxed `AsyncResponseTarget` trait objects across process boundaries, this series of commits changes `AsyncResponseTarget` to wrap a sender only. It is then the client's responsibility to spawn a thread to proxy calls from that sender to the consumer of the resource data. This only had to be done in a few places. In the future, we may want to collapse those threads into one per process to reduce overhead. (It is impossible to continue to use `AsyncResponseTarget` as a boxed trait object across processes, regardless of how much work is done on `ipc-channel`. Vtables are fundamentally incompatible with IPC across mutually untrusting processes.) In general, I was pretty pleased with how this turned out. The main changes are adding serialization functionality to various objects that `serde` does not know how to serialize natively—the most complicated being Hyper objects—and reworking `AsyncResponseTarget`. The overall structure of the code is unchanged, and other than `AsyncResponseTarget` no functionality was lost in moving to serialization and IPC. r? @jdm <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6586) <!-- Reviewable:end -->
|
|
jdm
added
S-tests-failed
and removed
S-awaiting-review
S-blocked-on-external
S-needs-rebase
labels
|
|
|
Pretty sure that's coming from https://github.com/servo/servo/pull/6586/files#diff-016eef304e69674de212b7fbcd9330fdR284 . |
|
|
|
|
@bors-servo: r=jdm |
|
|
script: Make the resource task communication use IPC channels. This change makes Servo use serialized messages over IPC channels for resource loading. The goal is to make it easier to make Servo multiprocess in the future. This patch does not make Servo multiprocess now; there are many other channels that need to be changed to IPC before that can happen. It does introduce a dependency on https://github.com/serde-rs/serde and https://github.com/pcwalton/ipc-channel for the first time. At the moment, `ipc-channel` uses JSON for serialization. This is because serde does not yet have official support for bincode. When serde gains support for bincode, I'll switch to that. For now, however, the JSON encoding and decoding will constitute a significant performance regression in resource loading. To avoid having to send boxed `AsyncResponseTarget` trait objects across process boundaries, this series of commits changes `AsyncResponseTarget` to wrap a sender only. It is then the client's responsibility to spawn a thread to proxy calls from that sender to the consumer of the resource data. This only had to be done in a few places. In the future, we may want to collapse those threads into one per process to reduce overhead. (It is impossible to continue to use `AsyncResponseTarget` as a boxed trait object across processes, regardless of how much work is done on `ipc-channel`. Vtables are fundamentally incompatible with IPC across mutually untrusting processes.) In general, I was pretty pleased with how this turned out. The main changes are adding serialization functionality to various objects that `serde` does not know how to serialize natively—the most complicated being Hyper objects—and reworking `AsyncResponseTarget`. The overall structure of the code is unchanged, and other than `AsyncResponseTarget` no functionality was lost in moving to serialization and IPC. r? @jdm <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6586) <!-- Reviewable:end -->
pcwalton commentedJul 10, 2015
This change makes Servo use serialized messages over IPC channels for resource loading. The goal is to make it easier to make Servo multiprocess in the future. This patch does not make Servo multiprocess now; there are many other channels that need to be changed to IPC before that can happen. It does introduce a dependency on https://github.com/serde-rs/serde and https://github.com/pcwalton/ipc-channel for the first time.
At the moment,
ipc-channeluses JSON for serialization. This is because serde does not yet have official support for bincode. When serde gains support for bincode, I'll switch to that. For now, however, the JSON encoding and decoding will constitute a significant performance regression in resource loading.To avoid having to send boxed
AsyncResponseTargettrait objects across process boundaries, this series of commits changesAsyncResponseTargetto wrap a sender only. It is then the client's responsibility to spawn a thread to proxy calls from that sender to the consumer of the resource data. This only had to be done in a few places. In the future, we may want to collapse those threads into one per process to reduce overhead. (It is impossible to continue to useAsyncResponseTargetas a boxed trait object across processes, regardless of how much work is done onipc-channel. Vtables are fundamentally incompatible with IPC across mutually untrusting processes.)In general, I was pretty pleased with how this turned out. The main changes are adding serialization functionality to various objects that
serdedoes not know how to serialize natively—the most complicated being Hyper objects—and reworkingAsyncResponseTarget. The overall structure of the code is unchanged, and other thanAsyncResponseTargetno functionality was lost in moving to serialization and IPC.r? @jdm