Merge pull request #754 from splitrb/reduce-more-redis-roundtrips

Reduce Redis roundtrips on the ab_test path

Author: andrehjr

lib/split/experiment.rb

@@ -102,7 +102,7 @@ def validate!
     end
 
     def new_record?
-      !@redis_storage.exists?
+      @redis_storage.new_record?
     end
 
     def ==(obj)
@@ -462,13 +462,16 @@ def persist_experiment_configuration
         else
           delete_metadata
         end
+
+        @redis_storage.reload
       end
 
       def remove_experiment_configuration
         @alternatives.each(&:delete)
         goals_collection.delete
         delete_metadata
         redis.del(@name)
+        @redis_storage.reload
       end
 
       def experiment_configuration_has_changed?

lib/split/experiment_storage.rb

@@ -13,6 +13,14 @@ def load
         @data ||= load!
       end
 
+      def reload
+        @data = nil
+      end
+
+      def new_record?
+        load[:alternatives].empty?
+      end
+
       def load!
         experiment_config = load_experiment
         alternatives = load_alternatives

lib/split/user.rb

@@ -15,13 +15,25 @@ def initialize(context, adapter = nil)
 
     def cleanup_old_experiments!
       return if @cleaned_up
-      keys_without_finished(user.keys).each do |key|
-        experiment = Experiment.new key_without_version(key)
-        if experiment.nil? || experiment.has_winner? || experiment.start_time.nil?
-          user.delete key
-          user.delete Experiment.finished_key(key)
+      keys = keys_without_finished(user.keys)
+      names = keys.map { |key| key_without_version(key) }
+
+      unless names.empty?
+        winners, start_times = Split.redis.pipelined do |pipe|
+          pipe.hmget(:experiment_winner, *names)
+          pipe.hmget(:experiment_start_times, *names)
+        end
+
+        keys.each_with_index do |key, index|
+          has_winner = !winners[index].nil?
+          not_started = start_times[index].nil?
+          if has_winner || not_started
+            user.delete key
+            user.delete Experiment.finished_key(key)
+          end
         end
       end
+
       @cleaned_up = true
     end
 
@@ -43,10 +55,17 @@ def cleanup_old_versions!(experiment)
     end
 
     def active_experiments
+      experiments_by_key = keys_without_finished(user.keys).each_with_object({}) do |key, memo|
+        memo[key] = Metric.possible_experiments(key_without_version(key))
+      end
+
+      names = experiments_by_key.values.flatten.map(&:name).uniq
+      winners = names.empty? ? {} : names.zip(Split.redis.hmget(:experiment_winner, *names)).to_h
+
       experiment_pairs = {}
-      keys_without_finished(user.keys).each do |key|
-        Metric.possible_experiments(key_without_version(key)).each do |experiment|
-          if !experiment.has_winner?
+      experiments_by_key.each do |key, experiments|
+        experiments.each do |experiment|
+          unless winners[experiment.name]
             experiment_pairs[key_without_version(key)] = user[key]
           end
         end

spec/support/redis_call_counter.rb

@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+require "redis-client"
+
+module SplitRedisCallInstrumentation
+  KEY = :split_redis_call_count
+
+  def call(command, config)
+    SplitRedisCallInstrumentation.increment
+    super
+  end
+
+  def call_pipelined(commands, config)
+    SplitRedisCallInstrumentation.increment
+    super
+  end
+
+  class << self
+    def count
+      previous = Thread.current[KEY]
+      Thread.current[KEY] = 0
+      yield
+      Thread.current[KEY]
+    ensure
+      Thread.current[KEY] = previous
+    end
+
+    def increment
+      count = Thread.current[KEY]
+      Thread.current[KEY] = count + 1 if count
+    end
+  end
+end
+
+RedisClient.register(SplitRedisCallInstrumentation)
+
+RSpec::Matchers.define :make_redis_calls do |expected|
+  supports_block_expectations
+
+  match do |block|
+    @actual = SplitRedisCallInstrumentation.count(&block)
+    values_match?(expected, @actual)
+  end
+
+  failure_message do
+    "expected block to make #{description_of(expected)} Redis roundtrip(s), but made #{@actual}"
+  end
+
+  failure_message_when_negated do
+    "expected block not to make #{description_of(expected)} Redis roundtrip(s), but it did (#{@actual})"
+  end
+
+  description do
+    "make #{description_of(expected)} Redis roundtrip(s)"
+  end
+end

spec/user_spec.rb

@@ -88,6 +88,55 @@
         @subject.cleanup_old_experiments!
       end
     end
+
+    context "with many experiments" do
+      let(:user_keys) do
+        {
+          "with_winner" => "red",
+          "not_started" => "red",
+          "active" => "red"
+        }
+      end
+
+      before do
+        with_winner = Split::ExperimentCatalog.find_or_create("with_winner", "red", "blue")
+        with_winner.start
+        with_winner.winner = "red"
+
+        Split::ExperimentCatalog.find_or_create("active", "red", "blue").start
+      end
+
+      it "keeps active experiments while dropping finished/not-started ones" do
+        @subject.cleanup_old_experiments!
+
+        expect(@subject.keys).to eq(["active"])
+      end
+
+      it "batches the winner/start-time lookups into a single roundtrip" do
+        expect { @subject.cleanup_old_experiments! }.to make_redis_calls(1)
+      end
+    end
+  end
+
+  context "#active_experiments" do
+    let(:user_keys) { { "with_winner" => "red", "active" => "red" } }
+
+    before do
+      with_winner = Split::ExperimentCatalog.find_or_create("with_winner", "red", "blue")
+      with_winner.start
+      with_winner.winner = "red"
+
+      Split::ExperimentCatalog.find_or_create("active", "red", "blue").start
+    end
+
+    it "excludes experiments that already have a winner" do
+      expect(@subject.active_experiments).to eq("active" => "red")
+    end
+
+    it "fetches every experiment's winner in a single call" do
+      expect(Split.redis).to receive(:hmget).with(:experiment_winner, any_args).once.and_call_original
+      @subject.active_experiments
+    end
   end
 
   context "allows user to be loaded from adapter" do