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

Move async execution from celluloid to concurrent-ruby #22934

Merged
merged 4 commits into from Jan 8, 2016

Conversation

Projects
None yet
@mperham
Contributor

mperham commented Jan 5, 2016

This removes 8 runtime gem dependencies from Rails:

Using hitimes 1.2.3
Using timers 4.1.1
Using celluloid-essentials 0.20.5
Using celluloid-extras 0.20.5
Using celluloid-fsm 0.20.5
Using celluloid-pool 0.20.5
Using celluloid-supervision 0.20.5
Using celluloid 0.17.2
Move async execution from celluloid to concurrent-ruby
This removes 8 runtime gem dependencies from Rails:

```
Using hitimes 1.2.3
Using timers 4.1.1
Using celluloid-essentials 0.20.5
Using celluloid-extras 0.20.5
Using celluloid-fsm 0.20.5
Using celluloid-pool 0.20.5
Using celluloid-supervision 0.20.5
Using celluloid 0.17.2
```
@rails-bot

This comment has been minimized.

rails-bot commented Jan 5, 2016

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

@matthewd

This comment has been minimized.

Member

matthewd commented Jan 5, 2016

r? @jeremy

❤️ 💚 💙 💛 💜

@rails-bot rails-bot assigned jeremy and unassigned pixeltrix Jan 5, 2016

end
def connection
Thread.current[:connection] || raise("No connection set")

This comment has been minimized.

@matthewd

matthewd Jan 5, 2016

Member

I guess this should probably use Module#thread_mattr_reader

@guilleiguaran

This comment has been minimized.

Member

guilleiguaran commented Jan 5, 2016

👏 👏 👏 👏 👏 👏 👏 👏

@mperham

This comment has been minimized.

Contributor

mperham commented Jan 5, 2016

Thanks @matthewd, updated.

@davydovanton

This comment has been minimized.

Contributor

davydovanton commented Jan 5, 2016

nice work @mperham! 👍

@@ -23,7 +23,7 @@ Gem::Specification.new do |s|
s.add_dependency 'coffee-rails', '~> 4.1.0'
s.add_dependency 'faye-websocket', '~> 0.10.0'
s.add_dependency 'websocket-driver', '~> 0.6.1'
s.add_dependency 'celluloid', '~> 0.17.2'
s.add_dependency 'concurrent-ruby', '~> 1.0.0'

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 6, 2016

Member

We can 🔥 this line since Active Support already depends in concurrent-ruby and will be depending for a long time. This way it would be easier to change the dependency requirement in the framework without having to remember to change in two places.

define_callbacks :work
include ActiveRecordConnectionManagement
def initialize(max_size=5)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 6, 2016

Member

neat pick 💅, max_size: 5

This comment has been minimized.

@mperham

mperham Jan 6, 2016

Contributor

It's "nit pick" but I like "neat pick" too! 😆

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 6, 2016

Member

lol! Yeah that one is good too

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Jan 6, 2016

@mperham

This comment has been minimized.

Contributor

mperham commented Jan 6, 2016

I investigated whether we could remove EventMachine too; it's a large native library with (from what I hear) an ugly C++ codebase. Unfortunately it's sunk in a bit deeper than Celluloid. We could investigate using celluloid-redis + nio4r + celluloid-io but that's a lot more work without a clear payoff from the start.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Jan 6, 2016

@mperham the author of faye-websocket suggested using websocket-driver directly as a way of eliminating the runtime dependency on EventMachine.

@dhh

This comment has been minimized.

Member

dhh commented Jan 6, 2016

Awesome, @mperham! We'll examine and benchmark this against what we currently have. If it flies for Basecamp 3, then it can go straight in.

Please do move ahead on the EventMachine extraction as well, if you can!

@searls

This comment has been minimized.

Contributor

searls commented Jan 6, 2016

Just wanted to chime in to say thanks to @mperham for taking the time to look into this—when I speculated about it a couple months ago I assumed it'd be a much more involved change than you managed to make it. Great work!

@mperham

This comment has been minimized.

Contributor

mperham commented Jan 7, 2016

Regarding removing EventMachine, here's what I've learned with a morning of research:

  1. tubesock is a simple websocket impl but it spins up two Threads per connection. I immediately discounted it for that reason.
  2. websocket-driver is a lower-level project which implements websockets and allows you to plug into an I/O system. it looks good, light and pluggable.
  3. faye-websocket-ruby plugs websocket-driver into EventMachine.
  4. reel plugs it into Celluloid and its ecosystem. nio4r is the gem doing the hard work here: it provides event-driven IO.
  5. there's no good, non-EM solution for an evented Redis connection. celluloid-redis pulls in all of Celluloid.

Given my limited knowledge, the ideal system would:

  1. Pull nio4r and websocket-driver into actioncable. Both are MIT licensed but I'm not clear how well they are supported.
  2. Write the event loop and glue code in actioncable which ties the two together. I don't know how much code or time this might take.
  3. Investigate how to integrate a Redis pubsub connection into the event loop.
  4. Profit?

That's a lot of uncertainty and hard work, likely weeks.

@dhh

This comment has been minimized.

Member

dhh commented Jan 8, 2016

Pratik had a chance to review the general approach and everything seems sound. We'll do the load testing next week, but I don't think that should hold up the merge. Whatever issues that may arise, we can deal with them subsequently.

dhh added a commit that referenced this pull request Jan 8, 2016

Merge pull request #22934 from mperham/master
Move async execution from celluloid to concurrent-ruby

@dhh dhh merged commit 3b7ccad into rails:master Jan 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nynhex

This comment has been minimized.

nynhex commented Jan 8, 2016

This is a good merge. @dhh when you mention load testing, are you testing against basecamp3? Is there any tests/load simulation I can help run?

@mperham

This comment has been minimized.

Contributor

mperham commented Jan 8, 2016

/play vuvuzela

@dhh

This comment has been minimized.

Member

dhh commented Jan 8, 2016

Yes, this is against Basecamp 3. We did a bunch of load testing early on
with Action Cable to satisfy our targets. They aren't easily extractable,
though. It'd be great to have a generic performance test suite, though!

On Fri, Jan 8, 2016 at 6:29 PM, James notifications@github.com wrote:

This is a good merge. @dhh https://github.com/dhh when you mention load
testing, are you testing against basecamp3? Is there any tests/load
simulation I can help run?


Reply to this email directly or view it on GitHub
#22934 (comment).

@nynhex

This comment has been minimized.

nynhex commented Jan 8, 2016

Ok, I'd be happy to help.

@dhh

This comment has been minimized.

Member

dhh commented Jan 8, 2016

Please do take a swing at that! This would be helpful in benchmarking different backends too. I know there's work progressing to use PostgreSQL instead of Redis, for example.

@nynhex

This comment has been minimized.

nynhex commented Jan 8, 2016

No problem, I'll start working on it this weekend. Glad to be of help. I love the work you guys are doing 👍

@evanphx

This comment has been minimized.

Contributor

evanphx commented Jan 8, 2016

@mperham Thank you my sweet prince. 👑

@dhh

This comment has been minimized.

Member

dhh commented Jan 8, 2016

Had to revert this for now in #22977 as it broke Action Cable as we were using it in Basecamp 3. @mperham, did you have a test app you were testing these changes in that sent communication both ways? I'll have us dig in next week to get this fixed and we can merge again.

@mperham

This comment has been minimized.

Contributor

mperham commented Jan 8, 2016

I didn't have a test app; I couldn't find anything in rails/rails that could be used for that purpose so I relied on the test suite. It's not clear to me what could have broken.

@dhh

This comment has been minimized.

Member

dhh commented Jan 8, 2016

Yeah, the test suite is not comprehensive enough at this point. Volunteers
desired for changing that fact!

No worries. It's probably something small. We'll take a stab at figuring it
out shortly.

On Fri, Jan 8, 2016 at 9:11 PM, Mike Perham notifications@github.com
wrote:

I didn't have a test app; I couldn't find anything in rails/rails that
could be used for that purpose so I relied on the test suite. It's not
clear to me what could have broken.


Reply to this email directly or view it on GitHub
#22934 (comment).

@maclover7

This comment has been minimized.

Member

maclover7 commented Jan 9, 2016

@dhh Just tested out @mperham's branch on https://github.com/maclover7/ac-test (code is from your AC intro video), and his code worked fine for me.

coffee-rails (~> 4.1.0)
concurrent-ruby (~> 1.0.0)

This comment has been minimized.

@arthurnn

arthurnn Jan 13, 2016

Member

Unfortunately sucker_punch depends on celluloid, so in our side(CI), we still need to install it. 😞

This comment has been minimized.

@mperham

mperham Jan 13, 2016

Contributor

Yep, true.

This removes 8 runtime gem dependencies ...

This comment has been minimized.

@jdantonio

jdantonio Jan 15, 2016

Contributor

@arthurnn sucker_punch is moving from Celluloid to concurrent-ruby. The in-progress work is here. I think @brandonhilkert will be releasing a beta fairly soon.

@jdantonio

This comment has been minimized.

Contributor

jdantonio commented Jan 15, 2016

@dhh @mperham Is there anything I can do to help? I've been traveling for work the past two weeks but I'll have much more time available starting next week. I'm happy to assist in any way I can.

@lifo

This comment has been minimized.

Member

lifo commented Jan 16, 2016

@mperham Thanks for working on this!! I figured out the issue w/ the PR. https://github.com/rails/rails/blob/master/actioncable/lib/action_cable/server/base.rb#L42 this needs to look like:

@worker_pool ||= ActionCable::Server::Worker.new(max_size: config.worker_pool_size)
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jan 16, 2016

Is there any way to test that in our test suite?

@dhh

This comment has been minimized.

Member

dhh commented Jan 16, 2016

This is now available in master again following @lifo's fix. Would be great to get some help fleshing out the test suite!

@jeremy jeremy referenced this pull request Feb 1, 2016

Open

Concurrent-edge exploration #1

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