Efficiency issues when subscribing to classes

[MarkMT]

Recently while chasing down some database performance bottlenecks in my application, I noticed a fairly significant amount of time being spent broadcasting events created by wisper-activerecord. It turns out that the underlying issue concerns wisper itself...

My application involves subscribing to a number of classes, each along the lines of

MyPublisher.subscribe(my_listener)

where the publisher is an activerecord model. Wisper handles these cases by instantiating an ObjectRegistration for each listener and adding each one to a set of global_registrations that are stored in the GlobalListener class and accessible by all publishers.

When broadcast is called on an activerecord instance, e.g. in one of the AR callbacks defined by wisper-activerecord, a method of the same name is called on all registrations accessible by the model, which in my case are all global_registrations (I have no instance-level local_registrations or temporary_registrations). That method involves first testing those registrations to see whether the specified event should be broadcast:

def broadcast(event, publisher, *args)
  method_to_call = map_event_to_method(event)
  if should_broadcast?(event) && listener.respond_to?(method_to_call) && publisher_in_scope?(publisher)
    broadcaster.broadcast(listener, publisher, method_to_call, args)
  end
end

Even though the callback is triggered by a CRUD event on a single AR record of known class, this test is performed for every global_registration created for every AR publisher class. In my case I have around 20 publisher classes and each one has multiple listeners, so the total number of global_registrations to be tested each time is potentially of the order of 100.

The computational cost of this process is dominated by the method publisher_in_scope? :

def publisher_in_scope?(publisher)
  allowed_classes.empty? || publisher.class.ancestors.any? { |ancestor| allowed_classes.include?(ancestor.to_s) }
end

When used in the context of wisper-activerecord, publisher.class.ancestors includes all of the modules included in ActiveRecord::Base and its parent classes. In my case that amounts to 80+ items.

Because of a particular pressing need I had to eliminate unnecessary computation, the approach I took to address this situation involved:

1. creating a new set of class-specific registrations represented by the class instance variable @class_registrations and overriding the class-level subscribe method to add registrations to this new set rather than to global_registrations. This significantly reduces the number of registrations that need to be tested each time an event is broadcast. The new definitions are wrapped in a Publisher module that I include into my AR models instead of Wisper.model.
2. overriding Wisper::ObjectRegistration#publisher_in_scope? to eliminate the testing of ancestor modules. To ensure that we're able to pick up registrations defined on any parent classes, a new class method registrations recursively retrieves registrations from any of the publisher's superclasses.

The approach is illustrated in the following gist - https://gist.github.com/MarkMT/a983fd7c23414fb28de3e883c24af309

Obviously this isn't a general solution since it doesn't touch the gem directly, but it should serve to highlight the issue I'm raising and to indicate broadly one way of addressing it should someone wish to do so. Note that the gist includes the definition of Wisper::ObjectRegistration#broadcast only for the sake of clarity (for me) to provide context and to add some comments. The method itself is unchanged.

[krisleech]

Thanks for the detailed explanation, very helpful.

Without having given this much thought (yet) I'm not sure how this would be improved without removing features. Any suggestions welcome.

I do find it surprising that publisher.class.ancestors.any? { |ancestor| allowed_classes.include?(ancestor.to_s) is slow even with 100's of elements. Maybe we can come up with a benchmark to simulate this to figure out what/why it is slow.

[moiristo]

We noticed this today when adding scopes to our listeners. Publishers are ActiveRecord models. Our test suite ran 2-3 times slower than normal. I added the following (AR specific) monkeypatch:

module Wisper
  class ObjectRegistration < Registration
    module ScopeHelper
      def publisher_in_scope?(publisher)
        if publisher.class.respond_to?(:base_class)
          allowed_classes.empty? || allowed_classes.include?(publisher.class.base_class.to_s)
        else
          super
        end
      end
    end

    prepend ScopeHelper
  end
end

Simple local testing yielded results around 36ms for the original implementation and 0.02ms for the implementation using publisher.class.base_class. I'm wondering if the use of ancestors isn't wrong to begin with, as it includes all modules that have been included in a class as well (I tested with an AR model consisting of 117 ancestors). I'd happily define all subclasses/modules in allowed_classes instead to maximize performance. I don't know what the use case was for it in the first place though.