From 1c8b88baa55bdba8f3d58e7bf2b48af83b97593f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Luis=20Leal=20Cardoso=20Junior?= Date: Sat, 25 Jul 2026 23:09:53 -0300 Subject: [PATCH 1/3] Drop redundant exists? roundtrip in Experiment#save --- lib/split/experiment.rb | 5 ++++- lib/split/experiment_storage.rb | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb index 3820a0cc..042a2a4a 100644 --- a/lib/split/experiment.rb +++ b/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,6 +462,8 @@ def persist_experiment_configuration else delete_metadata end + + @redis_storage.reload end def remove_experiment_configuration @@ -469,6 +471,7 @@ def remove_experiment_configuration goals_collection.delete delete_metadata redis.del(@name) + @redis_storage.reload end def experiment_configuration_has_changed? diff --git a/lib/split/experiment_storage.rb b/lib/split/experiment_storage.rb index 96bb76fa..382ad765 100644 --- a/lib/split/experiment_storage.rb +++ b/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 From c9b54c8294492bbac689928372826897044dd1ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Luis=20Leal=20Cardoso=20Junior?= Date: Sun, 26 Jul 2026 09:32:43 -0300 Subject: [PATCH 2/3] Batch cleanup_old_experiments! into one pipelined roundtrip --- lib/split/user.rb | 22 +++++++++--- spec/support/redis_call_counter.rb | 56 ++++++++++++++++++++++++++++++ spec/user_spec.rb | 28 +++++++++++++++ 3 files changed, 101 insertions(+), 5 deletions(-) create mode 100644 spec/support/redis_call_counter.rb diff --git a/lib/split/user.rb b/lib/split/user.rb index 7d9b90c1..170a8d35 100644 --- a/lib/split/user.rb +++ b/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 diff --git a/spec/support/redis_call_counter.rb b/spec/support/redis_call_counter.rb new file mode 100644 index 00000000..fc43dfdf --- /dev/null +++ b/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 diff --git a/spec/user_spec.rb b/spec/user_spec.rb index 23f5e5c6..a0bcdbff 100644 --- a/spec/user_spec.rb +++ b/spec/user_spec.rb @@ -88,6 +88,34 @@ @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 "allows user to be loaded from adapter" do From 92490bc90994004a4c0b5d6e84c17282f0d8eb2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Luis=20Leal=20Cardoso=20Junior?= Date: Sun, 26 Jul 2026 09:36:08 -0300 Subject: [PATCH 3/3] Batch active_experiments winner lookups into one roundtrip --- lib/split/user.rb | 13 ++++++++++--- spec/user_spec.rb | 21 +++++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/lib/split/user.rb b/lib/split/user.rb index 170a8d35..b7b32c73 100644 --- a/lib/split/user.rb +++ b/lib/split/user.rb @@ -55,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 diff --git a/spec/user_spec.rb b/spec/user_spec.rb index a0bcdbff..5e46210f 100644 --- a/spec/user_spec.rb +++ b/spec/user_spec.rb @@ -118,6 +118,27 @@ 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 it "loads user from adapter (RedisAdapter)" do user = Split::Persistence::RedisAdapter.new(nil, 112233)