Merge pull request #575 from gnanou/combined_experiment_performance

Combined experiment performance improvements

Author: andrehjr

lib/split/experiment.rb

@@ -81,12 +81,12 @@ def save
 
       if new_record?
         start unless Split.configuration.start_manually
+        persist_experiment_configuration
       elsif experiment_configuration_has_changed?
         reset unless Split.configuration.reset_manually
+        persist_experiment_configuration
       end
 
-      persist_experiment_configuration if new_record? || experiment_configuration_has_changed?
-
       redis.hset(experiment_config_key, :resettable, resettable)
       redis.hset(experiment_config_key, :algorithm, algorithm.to_s)
       self
@@ -144,11 +144,13 @@ def winner
     end
 
     def has_winner?
-      !winner.nil?
+      return @has_winner if defined? @has_winner
+      @has_winner = !winner.nil?
     end
 
     def winner=(winner_name)
       redis.hset(:experiment_winner, name, winner_name.to_s)
+      @has_winner = true
     end
 
     def participant_count
@@ -161,6 +163,7 @@ def control
 
     def reset_winner
       redis.hdel(:experiment_winner, name)
+      @has_winner = false
     end
 
     def start

lib/split/user.rb

@@ -8,16 +8,19 @@ class User
 
     def initialize(context, adapter=nil)
       @user = adapter || Split::Persistence.adapter.new(context)
+      @cleaned_up = false
     end
 
     def cleanup_old_experiments!
+      return if @cleaned_up
       keys_without_finished(user.keys).each do |key|
         experiment = ExperimentCatalog.find key_without_version(key)
         if experiment.nil? || experiment.has_winner? || experiment.start_time.nil?
           user.delete key
           user.delete Experiment.finished_key(key)
         end
       end
+      @cleaned_up = true
     end
 
     def max_experiments_reached?(experiment_key)

spec/dashboard_spec.rb

@@ -142,7 +142,7 @@ def link(color)
     it "removes winner" do
       post "/reopen?experiment=#{experiment.name}"
 
-      expect(experiment).to_not have_winner
+      expect(Split::ExperimentCatalog.find(experiment.name)).to_not have_winner
     end
 
     it "keeps existing stats" do

spec/experiment_spec.rb

@@ -213,12 +213,41 @@ def alternative(color)
     it "should have no winner initially" do
       expect(experiment.winner).to be_nil
     end
+  end
 
+  describe 'winner=' do
     it "should allow you to specify a winner" do
       experiment.save
       experiment.winner = 'red'
       expect(experiment.winner.name).to eq('red')
     end
+
+    context 'when has_winner state is memoized' do
+      before { expect(experiment).to_not have_winner }
+
+      it 'should keep has_winner state consistent' do
+        experiment.winner = 'red'
+        expect(experiment).to have_winner
+      end
+    end
+  end
+
+  describe 'reset_winner' do
+    before { experiment.winner = 'green' }
+
+    it 'should reset the winner' do
+      experiment.reset_winner
+      expect(experiment.winner).to be_nil
+    end
+
+    context 'when has_winner state is memoized' do
+      before { expect(experiment).to have_winner }
+
+      it 'should keep has_winner state consistent' do
+        experiment.reset_winner
+        expect(experiment).to_not have_winner
+      end
+    end
   end
 
   describe 'has_winner?' do
@@ -235,6 +264,12 @@ def alternative(color)
         expect(experiment).to_not have_winner
       end
     end
+
+    it 'memoizes has_winner state' do
+      expect(experiment).to receive(:winner).once
+      expect(experiment).to_not have_winner
+      expect(experiment).to_not have_winner
+    end
   end
 
   describe 'reset' do

spec/user_spec.rb

@@ -59,6 +59,17 @@
         expect(@subject.keys).to include("link_color:finished")
       end
     end
+
+    context 'when already cleaned up' do
+      before do
+        @subject.cleanup_old_experiments!
+      end
+
+      it 'does not clean up again' do
+        expect(@subject).to_not receive(:keys_without_finished)
+        @subject.cleanup_old_experiments!
+      end
+    end
   end
 
   context "instantiated with custom adapter" do