Reduce the Dashboard roundtrips for alternatives and calculation

lib/split/alternative.rb

@@ -180,13 +180,13 @@ def delete
       Split.redis.del(key)
     end
 
+    def key
+      "#{experiment_name}:#{name}"
+    end
+
     private
       def hash_with_correct_values?(name)
         Hash === name && String === name.keys.first && Float(name.values.first) rescue false
       end
-
-      def key
-        "#{experiment_name}:#{name}"
-      end
   end
 end

lib/split/dashboard/experiment.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "forwardable"
+require "json"
 
 module Split
   class DashboardExperiment
@@ -10,9 +11,7 @@ def initialize(experiment, goal = nil)
     end
 
     def alternatives
-      @alternatives ||= @experiment.alternatives.map do |alternative|
-        DashboardAlternative.new(alternative, @experiment, @goal)
-      end
+      @alternatives ||= build_alternatives
     end
 
     def extra_columns
@@ -37,48 +36,82 @@ def total_unfinished
     def total_completed
       alternatives.sum(&:completed)
     end
+
+    private
+      def build_alternatives
+        alternatives = @experiment.alternatives
+        stored = Split.redis.pipelined do |pipe|
+          alternatives.each { |alternative| pipe.hgetall(alternative.key) }
+        end
+
+        views = alternatives.zip(stored).map do |alternative, data|
+          DashboardAlternative.new(alternative, @experiment, @goal, data)
+        end
+        views.each { |view| view.control = views.first }
+      end
   end
 
   class DashboardAlternative
     extend Forwardable
     def_delegators :@alternative, :name, :control?
 
-    attr_reader :participants, :unfinished, :completed, :extra_info
+    attr_reader :participants, :unfinished, :completed, :extra_info, :p_winner
+    attr_writer :control
 
-    def initialize(alternative, experiment, goal = nil)
+    def initialize(alternative, experiment, goal, data)
       @alternative = alternative
-      @experiment = experiment
-      @goal = goal
 
-      @participants = alternative.participant_count
-      @unfinished = alternative.unfinished_count
-      @completed = alternative.completed_count(goal)
-      @extra_info = alternative.extra_info
+      @participants = data["participant_count"].to_i
+      @completed = data[alternative.set_field(goal)].to_i
+      @unfinished = @participants - all_completed(data, experiment.goals)
+      @p_winner = data[alternative.set_prob_field(goal)].to_f
+      @extra_info = parse_extra_info(data["recorded_info"])
     end
 
     def conversion_rate
-      @conversion_rate ||= @alternative.conversion_rate(@goal)
+      return 0 if participants.zero?
+
+      completed.to_f / participants
     end
 
     def conversion_delta
       return if control?
 
-      control_rate = @experiment.control.conversion_rate(@goal)
+      control_rate = @control.conversion_rate
       return if control_rate <= 0 || conversion_rate == control_rate
 
       conversion_rate / control_rate - 1
     end
 
     def z_score
-      @z_score ||= @alternative.z_score(@goal)
-    end
+      return "N/A" if control?
 
-    def p_winner
-      @p_winner ||= @alternative.p_winner(@goal)
+      rate = conversion_rate
+      control_rate = @control.conversion_rate
+      return "N/A" if rate > 1 || control_rate > 1
+
+      Split::Zscore.calculate(rate, participants, control_rate, @control.participants)
     end
 
     def winner?
-      @experiment.winner.name == name
+      @alternative.experiment.winner.name == name
     end
+
+    private
+      def all_completed(data, goals)
+        goals.reduce(data[@alternative.set_field(nil)].to_i) do |sum, goal|
+          sum + data[@alternative.set_field(goal)].to_i
+        end
+      end
+
+      def parse_extra_info(recorded_info)
+        return {} unless recorded_info && recorded_info.length > 1
+
+        begin
+          JSON.parse(recorded_info)
+        rescue
+          {}
+        end
+      end
   end
 end

lib/split/experiment.rb

@@ -288,31 +288,30 @@ def load_from_redis
 
     def can_calculate_winning_alternatives?
       self.alternatives.all? do |alternative|
-        alternative.participant_count >= 0 &&
-        (alternative.participant_count >= alternative.completed_count)
+        participants = alternative.participant_count
+        participants >= 0 && participants >= alternative.completed_count
       end
     end
 
     def calc_winning_alternatives
-      return unless can_calculate_winning_alternatives?
-
       # Cache the winning alternatives so we recalculate them once per the specified interval.
       intervals_since_epoch =
         Time.now.utc.to_i / Split.configuration.winning_alternative_recalculation_interval
 
-      if self.calc_time != intervals_since_epoch
-        if goals.empty?
-          self.estimate_winning_alternative
-        else
-          goals.each do |goal|
-            self.estimate_winning_alternative(goal)
-          end
+      return if self.calc_time == intervals_since_epoch
+      return unless can_calculate_winning_alternatives?
+
+      if goals.empty?
+        self.estimate_winning_alternative
+      else
+        goals.each do |goal|
+          self.estimate_winning_alternative(goal)
         end
+      end
 
-        self.calc_time = intervals_since_epoch
+      self.calc_time = intervals_since_epoch
 
-        self.save
-      end
+      self.save
     end
 
     def estimate_winning_alternative(goal = nil)

spec/dashboard/alternative_spec.rb

@@ -9,7 +9,7 @@
   let(:alternative) { experiment.alternatives[1] }
 
   def view_for(alternative)
-    Split::DashboardAlternative.new(alternative, experiment)
+    Split::DashboardExperiment.new(experiment).alternatives.find { |view| view.name == alternative.name }
   end
 
   describe "#conversion_delta" do

spec/dashboard/experiment_spec.rb

@@ -74,4 +74,46 @@
       expect(view.total_unfinished).to eq(27)
     end
   end
+
+  describe "Redis roundtrips" do
+    before do
+      blue.participant_count = 10
+      blue.set_completed_count(1)
+      blue.record_extra_info("revenue", 5)
+      red.participant_count = 20
+      red.set_completed_count(2)
+    end
+
+    it "loads every alternative in one roundtrip" do
+      expect { view.alternatives }.to make_redis_calls(1)
+    end
+
+    it "leaves the alternatives themselves reading straight from Redis" do
+      view.alternatives
+
+      expect { blue.participant_count }.to make_redis_calls(1)
+    end
+
+    it "reads a whole table without going back to Redis" do
+      view.alternatives
+
+      expect {
+        view.alternatives.each do |alternative|
+          alternative.participants
+          alternative.unfinished
+          alternative.completed
+          alternative.conversion_rate
+          alternative.conversion_delta
+          alternative.z_score
+          alternative.p_winner
+          alternative.extra_info
+        end
+        view.extra_columns
+        view.extra_totals
+        view.total_participants
+        view.total_unfinished
+        view.total_completed
+      }.to make_redis_calls(0)
+    end
+  end
 end

spec/experiment_spec.rb

@@ -592,5 +592,12 @@ def same_but_different_alternative
       expect(experiment.calc_winning_alternatives).not_to be nil
       expect(experiment.calc_winning_alternatives).to be nil
     end
+
+    it "reads only the recalculation timestamp when the interval has not elapsed" do
+      experiment = Split::ExperimentCatalog.find_or_create({ "link_color4" => ["purchase"] }, "blue", "red", "green")
+      experiment.calc_winning_alternatives
+
+      expect { experiment.calc_winning_alternatives }.to make_redis_calls(1)
+    end
   end
 end