Skip to content
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

ActionCable streaming performance #26999

Open
palkan opened this issue Nov 9, 2016 · 7 comments
Open

ActionCable streaming performance #26999

palkan opened this issue Nov 9, 2016 · 7 comments

Comments

@palkan
Copy link
Contributor

@palkan palkan commented Nov 9, 2016

The current implementation of streaming has a rather poor performance (of course, you've heard about websocket shootout).

One of the bottlenecks is the callback hell. Instead of simply broadcasting messages to sockets we create a callback wrapped in a callback wrapped in another callback etc.
And we also decode and encode message again for every socket.

Here is the benchmark I wrote to measure the performance. The results are following:

# Very simple transmitter: just broadcast message as is (and adds identifier)
  SimpleSubscriberMap   239.738  (±21.3%) i/s -      1.120k in   5.003338s

# Simple transmitter with JSON round-trip
DecodingSubscriberMap   227.131  (±19.4%) i/s -      1.083k in   5.041594s

# Current ActionCable implementation
        SubscriberMap     9.040  (±11.1%) i/s -     45.000  in   5.028229s

Websocket Shootout benchmark also shows that callback-less version (using this very simple patch) is performing much better (see slides).

So, I have a couple of proposals.

Soft Proposal

  • do not create useless callbacks when there are no user-specified handlers

  • do not decode/encode messages for each socket

Hard Proposal

Soft Proposal + ...

  • deprecate custom streaming callbacks, 'cause they influence broadcasting performance (and, IMO, they don't make any sense but provide overhead).

/cc @matthewd @maclover7

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 9, 2016

Soft Proposal

  • do not create useless callbacks when there are no user-specified handlers
  • do not decode/encode messages for each socket

Good idea.

Hard Proposal

  • deprecate custom streaming callbacks

That doesn't make much sense to me. The point of Action Cable is not to implement websocket connections in Ruby because it's fun -- it's to give the implementation access to the running Rails app. Making straight retransmission faster is worthwhile, but it's not the only valid use case.

@maclover7
Copy link
Member

@maclover7 maclover7 commented Nov 9, 2016

@palkan Thank you for opening up this issue! You've outlined some interesting performance proposals in the description -- thank you for taking the time to look into this. The best way forward is to start opening up PRs with patches, to get community and Rails team feedback.

@dhh
Copy link
Member

@dhh dhh commented Dec 30, 2016

When you say "custom streaming callbacks", you're talking about after_subscribed and friends in callbacks? I can see that these are used to power the periodic timers. And we also use one of the callbacks one time. But I think we can be more efficient with these and basically only enable them on a channel if the channel implements that method. If that's too hard, I'm actually also OK with having an explicit method to turn them on, like "uses_subscription_callbacks" or similar. Then channels that don't use that won't suffer the performance penalty.

@palkan
Copy link
Contributor Author

@palkan palkan commented Dec 30, 2016

@dhh
I meant these callbacks:

stream_from "notifications" do |msg|
  # do custom broadcasting
end

Maybe, interceptors could be a better naming.

ActiveModel-like callbacks do not cause any problems, of course)

Sorry for misunderstanding 😔

@dhh
Copy link
Member

@dhh dhh commented Dec 30, 2016

@NirvashPrime
Copy link

@NirvashPrime NirvashPrime commented Oct 8, 2018

I see this hasn't been updated in almost two years, is this still an issue?

@palkan
Copy link
Contributor Author

@palkan palkan commented Oct 8, 2018

is this still an issue?

Since nothing has changed in the way stream callbacks work it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.