-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Linux OsIpcReceiverSet - Switch to use mio #94
Conversation
798165b
to
6ed6eb6
Compare
Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions. src/platform/linux/mod.rs, line 418 [r1] (raw file):
Wasn't a huge fan of having to track all of the fd's being watched, but AFAIK since src/platform/linux/mod.rs, line 435 [r1] (raw file):
It would be reasonably easy to switch from using a slice to a dynamically sized vector. I'm not a huge fan of having a Comments from Reviewable |
ccf98d8
to
9719339
Compare
Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. src/platform/linux/mod.rs, line 418 [r1] (raw file):
|
I'd like to point out that although the platform backend is named I wonder whether there is something specifically motivating this change; or it just looked like it might be a good idea in general? Also, when doing performance optimisation, I suggest adding benchmark tests to keep track of actual results... |
Great points. So actually the reason why I opened this PR, was because I started porting this to FreeBSD for servo/servo#11625 and was going to use Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. src/platform/linux/mod.rs, line 418 [r1] (raw file):
|
Thanks for the comments! If you disagree with the Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. Comments from Reviewable |
Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. src/platform/linux/mod.rs, line 435 [r1] (raw file):
|
I do not exactly disagree with the More importantly, I think it is really important to find a sane approach for handling such minor variations in general. I can't speak for @pcwalton and others -- but I for my part would be very sad if we end up with a (Or a |
Call me insane, but why can't we just use mio with UNIX sockets? |
@notriddle can you elaborate please? Where exactly would |
mio provides an abstraction layer between async IO apis, such as epoll and kqueue. |
9719339
to
6c0e29b
Compare
@notriddle that might work I guess... (Though the Can't tell for sure, as most of |
After taking a quick peek at The Take this with a grain of salt, as this is my first or second PR to ipc. |
Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions. src/platform/linux/mod.rs, line 435 [r1] (raw file):
|
@danlrobertson well, in general, if there is an existing abstraction we can use, we probably should -- especially if it involves messy things like platform-specific and unsafe code... The problem is really just that from a quick glance at the |
Added some benchmarks for
I repeated it a few times and the results were consistent. Please double check my benchmark code... This was the first time I've written benchmarks in rust. Also I'm really curious if others get similar results. If you test it out on your boxen, I'd be interested to know your results. |
045daba
to
bf5f767
Compare
I just created a branch here attempting to use |
@@ -170,3 +170,51 @@ fn size_22_4m(b: &mut test::Bencher) { | |||
fn size_23_8m(b: &mut test::Bencher) { | |||
bench_size(b, 8 * 1024 * 1024); | |||
} | |||
|
|||
macro_rules! gen_poll_test { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I originally did for the other benchmark tests -- but @pcwalton didn't like the use of macros here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use a function instead
Whoops, forgot that you were using Reviewable here... Hope you don't mind. I'm not sure what to make of the disappointing |
bf5f767
to
5754013
Compare
So after the removal of the assert the results are more in line with what is expected. mio performs better than the original for the 1% case, but not as good as using epoll directly original
mio
epoll
AFAIK mio will not perform better than directly using epoll for our use case due to the way mio wraps epoll. It does use Note: using mio does perform better than our current implementation, so IMO |
5754013
to
b720d67
Compare
Linux OsIpcReceiverSet - Switch to use mio Switch from using poll to a level-triggered use of epoll for the Linux OSIpcReceiverSet. The use of epoll should perform better when ther are a large number of watched fd's. Side note: An edge-triggered use of `epoll` would be very easy here, but to make the transition easier I chose level-triggered. Let me know if you thing edge-triggered would be better. Comments and critiques are welcome! <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/ipc-channel/94) <!-- Reviewable:end -->
💔 Test failed - status-travis |
I guess another retry is in order? Or is this CI issue more permanent?... |
@bors-servo retry |
Linux OsIpcReceiverSet - Switch to use mio Switch from using poll to a level-triggered use of epoll for the Linux OSIpcReceiverSet. The use of epoll should perform better when ther are a large number of watched fd's. Side note: An edge-triggered use of `epoll` would be very easy here, but to make the transition easier I chose level-triggered. Let me know if you thing edge-triggered would be better. Comments and critiques are welcome! <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/ipc-channel/94) <!-- Reviewable:end -->
💔 Test failed - status-travis |
@bors-servo retry |
Linux OsIpcReceiverSet - Switch to use mio Switch from using poll to a level-triggered use of epoll for the Linux OSIpcReceiverSet. The use of epoll should perform better when ther are a large number of watched fd's. Side note: An edge-triggered use of `epoll` would be very easy here, but to make the transition easier I chose level-triggered. Let me know if you thing edge-triggered would be better. Comments and critiques are welcome! <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/ipc-channel/94) <!-- Reviewable:end -->
💔 Test failed - status-appveyor |
|
@bors-servo retry |
Linux OsIpcReceiverSet - Switch to use mio Switch from using poll to a level-triggered use of epoll for the Linux OSIpcReceiverSet. The use of epoll should perform better when ther are a large number of watched fd's. Side note: An edge-triggered use of `epoll` would be very easy here, but to make the transition easier I chose level-triggered. Let me know if you thing edge-triggered would be better. Comments and critiques are welcome! <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/ipc-channel/94) <!-- Reviewable:end -->
💔 Test failed - status-appveyor |
Switch from using poll to using mio for the Unix implementation of OSIpcReceiverSet. The use of mio should result in better performance when there are a larger number of watched descriptors.
78891e7
to
f6cefc5
Compare
@bors-servo r=pcwalton,antrik,emilio |
📌 Commit f6cefc5 has been approved by |
…emilio Linux OsIpcReceiverSet - Switch to use mio Switch from using poll to a level-triggered use of epoll for the Linux OSIpcReceiverSet. The use of epoll should perform better when ther are a large number of watched fd's. Side note: An edge-triggered use of `epoll` would be very easy here, but to make the transition easier I chose level-triggered. Let me know if you thing edge-triggered would be better. Comments and critiques are welcome! <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/ipc-channel/94) <!-- Reviewable:end -->
☀️ Test successful - status-appveyor, status-travis |
\o/ Thanks for all the comments and critiques in the reviews. |
Thank you for doing this, and for the patience to get this landed! |
Such a big change should have been tested on Servo. |
Switch from using poll to a level-triggered use of epoll for the Linux OSIpcReceiverSet. The use of epoll should perform better when ther are a large number of watched fd's.
Side note: An edge-triggered use of
epoll
would be very easy here, but to make the transition easier I chose level-triggered. Let me know if you thing edge-triggered would be better. Comments and critiques are welcome!This change is