Refactor ActionCable streaming by palkan · Pull Request #27044 · rails/rails

palkan · 2016-11-14T20:22:31Z

Implementation of #26999 proposals.

What have been done:

streaming handlers logic have been moved from streams.rb into separate classes
avoid JSON double-encoding

The benchmark show the following:

Calculating -------------------------------------
                     PR 268.290  (±23.9%) i/s -      1.200k in   5.036351s
             current     75.673  (±15.9%) i/s -    365.000  in   5.034415s

So, broadcasting is about 5-6x faster.

/cc @maclover7 @matthewd

maclover7

Looking very good so far 👍

Two small side notes:

Can you add a description of the changes (especially the performance improvements) to Action Cable's CHANGELOG.md?
Can you take a look at the failing Code Climate build?

maclover7 · 2016-11-14T20:58:16Z

actioncable/lib/action_cable/channel/streams_handlers/base.rb

+module ActionCable
+  module Channel
+    module StreamsHandlers
+      class Base # :nodoc:


Can we move Base's :nodoc: declaration up to StreamHandlers? Same deal with Custom.

maclover7 · 2016-11-14T20:58:50Z

actioncable/lib/action_cable/channel/streams_handlers/base.rb

+  module Channel
+    module StreamsHandlers
+      class Base # :nodoc:
+        attr_reader :channel, :identifier


I might be missing something, but is there are any reason why channel and identifier need to be part of the public API? 😬

jeremy · 2016-11-14T20:50:33Z

actioncable/lib/action_cable/channel/streams.rb

-
-        def identity_handler
-          -> message { message }
-        end


Refactoring these stream handler factory methods means they're no longer available for overriding in subclasses.

jeremy · 2016-11-14T20:52:47Z

actioncable/lib/action_cable/channel/streams.rb

+            ActionCable::Channel::StreamsHandlers::Custom.new(self, handler: user_handler, coder: coder)
          else
-            default_stream_handler broadcasting, coder: coder
+            ActionCable::Channel::StreamsHandlers::Base.new(self)


Should check that coder is JSON, too.

jeremy · 2016-11-14T21:04:15Z

actioncable/lib/action_cable/channel/streams_handlers/base.rb

+module ActionCable
+  module Channel
+    module StreamsHandlers
+      class Base # :nodoc:


This is a specialized no-op stream transmitter for use when there's no user handler and pubsub & transmit coders match. Wouldn't call it a Base transmitter which suggests it's meant for public API and subclassing.

jeremy · 2016-11-14T21:06:19Z

actioncable/lib/action_cable/channel/streams_handlers/custom.rb

+
+          alias encode decode
+        end
+      end


This is a no-op coder, hence the existing "identity" language. Introducing "null" language ambiguously suggests that it's coding as null/nil or doing null coding.

jeremy · 2016-11-14T21:07:02Z

actioncable/lib/action_cable/channel/streams_handlers/custom.rb

+        def call(message)
+          handler.(coder.decode(message))
+        end
+      end


This isn't inheriting anything substantial from Base—the inheritance relationship is misleading.

jeremy · 2016-11-14T21:08:28Z

actioncable/lib/action_cable/connection/base.rb

-        websocket.transmit encode(cable_message)
+      def transmit(cable_message, encoded = false) # :nodoc:
+        websocket.transmit(encoded ? cable_message : encode(cable_message))
      end


Boolean flags in method signature are a good red flag to investigate.

In this case, we really want a separate API to transmit already-encoded messages. The existing #transmit can invoke that API after encoding its message.

jeremy · 2016-11-14T21:09:20Z

actioncable/test/channel/stream_test.rb

+      when "custom"
+        DummyEncoder
+      when "none"
+        nil


This boilerplate isn't particular to this test; wouldn't move it.

jeremy · 2016-11-14T21:11:18Z

actioncable/lib/action_cable/channel/streams_handlers/base.rb

+        def call(message)
+          # We can skip message decoding and generate payload in a more simple way
+          connection.transmit(
+            "{\"identifier\":#{identifier.to_json},\"message\":#{message}}",


Should identifier be quoted here? It's an embedded JSON document.

No, we should call to_json (which adds surrounding quotes and escapes existing quotes), 'cause identifier is a JSON string itself.

jeremy · 2016-11-14T21:42:18Z

actioncable/lib/action_cable/channel/streams.rb

-          -> (message) do
-            transmit handler.(message), via: via
-          end
-        end


Could introduce coder-aware stream handling directly:

def stream_handler(broadcasting, user_handler, coder: nil) if user_handler user_stream_handler broadcasting, user_handler, coder: coder else default_stream_handler broadcasting, coder: coder end end def user_stream_handler(user_handler, coder: nil) -> encoded_message { user_handler.(coder.decode(encoded_message)) } end def default_stream_handler(broadcasting, coder: nil) coder ||= ActiveSupport::JSON via = "streamed from #{broadcasting}" if coder == ActiveSupport::JSON -> encoded_message { transmit_encoded encoded_message, coder: coder, via: via } else -> encoded_message { transmit coder.decode(encoded_message), via: via } end end

matthewd · 2016-11-14T23:35:12Z

I haven't yet looked in detail, but even in the no-op case, I wonder if we should decode to ensure it's valid JSON before we throw it at clients

palkan · 2016-11-15T08:13:11Z

I haven't yet looked in detail, but even in the no-op case, I wonder if we should decode to ensure it's valid JSON before we throw it at clients

No-op case doesn't mean that the message is JSON-encoded. For example:

ActionCable.server.broadcast "test", { x: 1 }, coder: MyYAMLCoder

No-op only requires that message is a string. Nothing more. Kind of stupid re-transmission.

Add Connection#transmit_encoded and Channel#transmit_encoded. Avoiding unnecessary decoding and logging.

palkan · 2016-11-15T12:01:55Z

@jeremy
Return steam handlers generation back to streams.rb. Looks more clear and concise, thanks!

Also, #transmit_encoded methods added to both Connection and Channel.
I intentionally skip logging and instrumentation. First, due to the overhead (adding logging decrease performance about twice!); secondly, I don't think that logging re-transmissions make any sense (we're logging broadcasting as a whole).

@maclover7
Changelog entry added.
Code Climate is green.

Preen · 2017-03-08T17:57:03Z

Is there any news on this?

rails-bot · 2019-12-18T12:52:51Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

Refactor AC streaming: extract handlers, avoid double-encoding

a23cb19

maclover7 added actioncable needs feedback labels Nov 14, 2016

maclover7 self-assigned this Nov 14, 2016

maclover7 suggested changes Nov 14, 2016

View reviewed changes

maclover7 added the needs work label Nov 14, 2016

jeremy requested changes Nov 14, 2016

View reviewed changes

Optimize default stream handlers

4eae47a

Add Connection#transmit_encoded and Channel#transmit_encoded. Avoiding unnecessary decoding and logging.

palkan force-pushed the ac/refactor-streaming branch from 50879f6 to 4eae47a Compare November 15, 2016 11:48

palkan mentioned this pull request Jan 11, 2017

Decouple ActionCable application logic from low-level logic #27648

Closed

rails-bot bot unassigned maclover7 Dec 18, 2019

rails-bot bot added the stale label Dec 18, 2019

rails-bot bot closed this Dec 25, 2019

Conversation

palkan commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maclover7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewd commented Nov 14, 2016

Uh oh!

palkan commented Nov 15, 2016

Uh oh!

palkan commented Nov 15, 2016

Uh oh!

Preen commented Mar 8, 2017

Uh oh!

rails-bot bot commented Dec 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

palkan commented Nov 14, 2016 •

edited

Loading

maclover7 left a comment •

edited

Loading