Always subscribe to event objects via `AS::Notifications.subscribe`

We don't need to have a special subscribe method for objects.  The
regular `subscribe` method is more expensive than a specialized method,
but `subscribe` should not be called frequently.  If that turns out to
be a hotspot, we can introduce a specialized method.  :)

3 files changed, 25 insertions, 21 deletions

diff --git a/activesupport/lib/active_support/notifications/fanout.rb b/activesupport/lib/active_support/notifications/fanout.rb
index 91d8491c7e..2fd683a1de 100644
--- a/activesupport/lib/active_support/notifications/fanout.rb
+++ b/activesupport/lib/active_support/notifications/fanout.rb
@@ -27,15 +27,6 @@ module ActiveSupport
         subscriber
       end
 
-      def subscribe_event(pattern = nil, &block)
-        subscriber = Subscribers.event_object_subscriber pattern, block
-        synchronize do
-          @subscribers << subscriber
-          @listeners_for.clear
-        end
-        subscriber
-      end
-
       def unsubscribe(subscriber_or_name)
         synchronize do
           case subscriber_or_name
@@ -80,12 +71,16 @@ module ActiveSupport
       module Subscribers # :nodoc:
         def self.new(pattern, listener)
           if listener.respond_to?(:start) && listener.respond_to?(:finish)
-            subscriber = Evented.new pattern, listener
+            subscriber_class = Evented
           else
-            subscriber = Timed.new pattern, listener
+            if listener.respond_to?(:arity) && listener.arity == 1
+              subscriber_class = EventObject
+            else
+              subscriber_class = Timed
+            end
           end
 
-          wrap_all pattern, subscriber
+          wrap_all pattern, subscriber_class.new(pattern, listener)
         end
 
         def self.event_object_subscriber(pattern, block)
diff --git a/activesupport/lib/active_support/notifications/instrumenter.rb b/activesupport/lib/active_support/notifications/instrumenter.rb
index 72e5064679..455b7a44a6 100644
--- a/activesupport/lib/active_support/notifications/instrumenter.rb
+++ b/activesupport/lib/active_support/notifications/instrumenter.rb
@@ -69,12 +69,14 @@ module ActiveSupport
         @allocation_count_finish = 0
       end
 
+      # Record information at the time this event starts
       def start!
         @time = now
         @cpu_time_start = now_cpu
         @allocation_count_start = now_allocations
       end
 
+      # Record information at the time this event finishes
       def finish!
         @cpu_time_finish = now_cpu
         @end = now
diff --git a/activesupport/test/notifications_test.rb b/activesupport/test/notifications_test.rb
index 119062d5bf..62817a839a 100644
--- a/activesupport/test/notifications_test.rb
+++ b/activesupport/test/notifications_test.rb
@@ -29,7 +29,7 @@ module Notifications
   class SubscribeEventObjects < TestCase
     def test_subscribe_events
       events = []
-      @notifier.subscribe_event do |event|
+      @notifier.subscribe do |event|
         events << event
       end
 
@@ -42,16 +42,23 @@ module Notifications
       assert_operator event.duration, :>, 0
     end
 
-    def test_subscribe_via_subscribe_method
-      events = []
-      @notifier.subscribe do |event|
-        events << event
+    def test_subscribe_via_top_level_api
+      old_notifier = ActiveSupport::Notifications.notifier
+      ActiveSupport::Notifications.notifier = ActiveSupport::Notifications::Fanout.new
+
+      event = nil
+      ActiveSupport::Notifications.subscribe("foo") do |e|
+        event = e
       end
 
-      ActiveSupport::Notifications.instrument("foo")
-      event = events.first
-      assert event, "should have an event"
-      assert_operator event.allocations, :>, 0
+      ActiveSupport::Notifications.instrument("foo") do
+        100.times { Object.new } # allocate at least 100 objects
+      end
+
+      assert event
+      assert_operator event.allocations, :>=, 100
+    ensure
+      ActiveSupport::Notifications.notifier = old_notifier
     end
   end