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

feat: Implement `EventStream` using `mio` #105

Merged
merged 2 commits into from Jul 16, 2018

Conversation

Projects
None yet
4 participants
@hawkw
Copy link
Contributor

hawkw commented Jun 7, 2018

This branch changes the implementation of Stream for EventStream
to use mio and tokio_reactor to register interest in events on
the Inotify file descriptor with the reactor, rather than using
task::current().notify() to unpark the task in the executor when
polling the fd would block.

Since the reactor is intended to drive all IO resources (such as the
inotify FD) and to be the bridge between tasks in the executor and
events from the OS, using PollEvented is a more correct way to
implement the stream.

This branch shouldn't result in any noticeable changes in behaviour,
but will hopefully allow tokio to manage Inotify's IO events more
intelligently.

@hannobraun

This comment has been minimized.

Copy link
Member

hannobraun commented Jun 8, 2018

Thanks for the PR! I'll take a closer look once #104 has been merged.

@hawkw hawkw force-pushed the hawkw:eliza/mio branch 3 times, most recently from 9903cdd to 2c63299 Jun 8, 2018

@hannobraun

This comment has been minimized.

Copy link
Member

hannobraun commented Jul 3, 2018

Now that #104 is merged, this PR is no longer blocked.

@hawkw I'm a bit clueless about mio/futures/tokio, so I'm not too confident in my ability to provide a good review. That said, I can't find anything wrong with this PR.

There's one thing that's keeping me from merging this right now: There's an open PR (#101) that is a follow-up to the initial futures work (#99). It was blocked back then (may still be?), so I couldn't merge right away. It's my impression that your PR supersedes #101. Is that correct?

@mathstuf This PR seems to supersede your open PR #101. Do you have any thoughts on this PR? I'm considering merging this PR and closing #101. Do you see any reason not to do that? Please note that this is based on a previous PR, and the first 4 commits are already merged. As of this writing, only fa6bf8a and 2c63299 are relevant.

hawkw added some commits Jun 7, 2018

feat: Implement `EventStream` using `mio`
This branch changes the implementation of `Stream` for `EventStream`
to use `mio` and `tokio_reactor` to register interest in events on
the Inotify file descriptor with the reactor, rather than using
`task::current().notify()` to unpark the task in the executor when
polling the fd would block.

Since the reactor is intended to drive all IO resources (such as the
inotify FD) and to be the bridge between tasks in the executor and
events from the OS, using `PollEvented` is a more correct way to
implement the stream.

This branch shouldn't result in any noticeable changes in behaviour,
but will hopefully allow tokio to manage Inotify's IO events more
intelligently.
refactor: Put `EventStream` in its own module
This commit moves `EventStream` to a separate file, since it adds a
lot of related code that's only used for implementing the stream.

Also, since the `EventStream` pulls in a lot of additional
dependencies, and some users may not be using `futures`/`tokio`, I've
added a feature flag, "stream", (on by default) so it can optionally
be disabled if it isn't used.

@hawkw hawkw force-pushed the hawkw:eliza/mio branch from 2c63299 to 548eafc Jul 3, 2018

@hawkw

This comment has been minimized.

Copy link
Contributor

hawkw commented Jul 3, 2018

First, just FYI, I've rebased this branch onto master, so the diff should no longer include diffs from PR #104.

@hannobraun

It's my impression that your PR supersedes #101. Is that correct?

Yeah, that's correct. I haven't looked too closely at #101, but here are some of the important differences from my perspective:

  • It looks like #101 uses the old version of tokio (i.e. the tokio-core and tokio-io crates), while this branch uses the new tokio crate. These are incompatible, and IMO since a majority of the ecosystem has migrated to the new tokio, it's better to use that.
  • On master, Inotify::event_stream method returns a concrete EventStream type. On #101, this changes to impl Stream<...>, while on this branch, we still return a named EventStream type. Personally, I think that while impl Trait is nice to use in applications or in internal code that's not part of a library's public API, it's not ideal for library methods to return an impl Trait, as it limits the way users of the library can use that returned type. For example, if I wanted to put the stream returned by Inotify::event_stream in a struct field, or use it as a type parameter to some other type, I'd have to box it. This could be a potentially breaking change.
  • #101 depends on the tokio_file_unixcrate, while my branch doesn't. I'm not familiar with that crate, and this isn't a value judgement, just a difference I noticed.
@hannobraun

This comment has been minimized.

Copy link
Member

hannobraun commented Jul 3, 2018

Thank you for the explanation, @hawkw, this is great! I'll give @mathstuf a few days to respond before making a decision, but so far everything looks great.

@hawkw

This comment has been minimized.

Copy link
Contributor

hawkw commented Jul 4, 2018

For what it's worth: we just found that switching to this branch from master fixed an issue where inotify-rs was causing our application to pin CPU use at 100%.

@olix0r

This comment has been minimized.

Copy link

olix0r commented Jul 4, 2018

In particular, this line is what causes this stream to burn CPU, since the executor can't leave this stream as parked (since the task is always notified).

hawkw added a commit to linkerd/linkerd2 that referenced this pull request Jul 4, 2018

proxy: Fix out-of-control inotify CPU use (#1263)
The `inotify-rs` library's `EventStream` implementation currently 
calls `task::current().notify()` in a hot loop when a poll returns
`WouldBlock`, causing the task to constantly burn CPU. 

This branch updates the `inotify-rs` dependency to point at a branch
of `inotify-rs` I had previously written. That branch  rewrites the 
`EventStream` to use `mio` to  register interest in the `inotify` file 
descriptor instead, fixing the out-of-control polling. 

When inotify-rs/inotify#105 is merged upstream, we can go back to 
depending on the master version of the library.

Fixes #1261

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hannobraun

This comment has been minimized.

Copy link
Member

hannobraun commented Jul 4, 2018

@olix0r Thank you for the info. If I remember correctly, we decided to land a preliminary version of futures support, as the more efficient version was blocked for various reasons.

Looking back, this seems like a mistake on my part. We should have documented this extensively and hidden it behind a Cargo feature, at the very least. I'm sorry for the trouble this caused for all of you!

@olix0r

This comment has been minimized.

Copy link

olix0r commented Jul 4, 2018

@hannobraun No worries! Luckily we caught this in testing ;)

Thanks for putting together this very useful library.

@hawkw

This comment has been minimized.

Copy link
Contributor

hawkw commented Jul 4, 2018

@hannobraun I think releasing the futures support experimentally was a good choice on your part, given that otherwise, we wouldn't have found this issue at all.

Honestly, I'm mostly just surprised nobody else hit this issue earlier, as I'm fairly sure could occur any time the EventStream was used (rather than being triggered by some particular uncommon condition). I guess nobody was using the futures support seriously enough to care about its performance until us.

By the way, thanks for being so willing to work with me on landing all the patches I've submitted, I really appreciate it!

@hannobraun

This comment has been minimized.

Copy link
Member

hannobraun commented Jul 5, 2018

@olix0r @hawkw Thank you both for your kind words!

I guess nobody was using the futures support seriously enough to care about its performance until us.

Yeah, probably. Judging from the downloads numbers on crates.io, this library must see a fair bit of use. Unfortunately, not a lot gets back to me.

By the way, thanks for being so willing to work with me on landing all the patches I've submitted, I really appreciate it!

No problem. Thank you for the patches!

olix0r pushed a commit to linkerd/linkerd2-proxy that referenced this pull request Jul 7, 2018

proxy: Fix out-of-control inotify CPU use (#1263)
The `inotify-rs` library's `EventStream` implementation currently 
calls `task::current().notify()` in a hot loop when a poll returns
`WouldBlock`, causing the task to constantly burn CPU. 

This branch updates the `inotify-rs` dependency to point at a branch
of `inotify-rs` I had previously written. That branch  rewrites the 
`EventStream` to use `mio` to  register interest in the `inotify` file 
descriptor instead, fixing the out-of-control polling. 

When inotify-rs/inotify#105 is merged upstream, we can go back to 
depending on the master version of the library.

Fixes #1261

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hannobraun

This comment has been minimized.

Copy link
Member

hannobraun commented Jul 16, 2018

I'm merging this. Sorry for the additional delay.

@hawkw Thank you again for your contributions!

@hannobraun hannobraun merged commit 1cd506e into inotify-rs:master Jul 16, 2018

2 checks passed

commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hawkw

This comment has been minimized.

Copy link
Contributor

hawkw commented Jul 16, 2018

@hannobraun

Sorry for the additional delay.

No problem at all!

@hawkw Thank you again for your contributions!

My pleasure! It was fun. :D Thanks for all your reviews.

hawkw added a commit to linkerd/linkerd2-proxy that referenced this pull request Jul 17, 2018

Point inotify dependency at master (#14)
Now that inotify-rs/inotify#105 has merged, we will no longer see
rampant CPU use from using the master version of `inotify`. I've 
updated Cargo.toml to depend on master rather than on my branch.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw added a commit to linkerd/linkerd2-proxy that referenced this pull request Jul 27, 2018

Point inotify dependency at master (#14)
Now that inotify-rs/inotify#105 has merged, we will no longer see
rampant CPU use from using the master version of `inotify`. I've 
updated Cargo.toml to depend on master rather than on my branch.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@mathstuf

This comment has been minimized.

Copy link
Contributor

mathstuf commented Aug 9, 2018

@mathstuf This PR seems to supersede your open PR #101. Do you have any thoughts on this PR?

Sorry, was on an extended vacation. Superseding is fine. This does look more idiomatic than my PR. Thanks @hawkw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment