From 472d17d1d01af6bb9d546a7d8219810b78cbd72d Mon Sep 17 00:00:00 2001 From: Mateusz Lata Date: Tue, 27 Jun 2023 17:52:57 +0200 Subject: [PATCH 1/6] Add test covering model without a scope. --- spec/performance_spec.rb | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 spec/performance_spec.rb diff --git a/spec/performance_spec.rb b/spec/performance_spec.rb new file mode 100644 index 0000000..d00ed1c --- /dev/null +++ b/spec/performance_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require "benchmark" +# TODO: Turn off performance-related tests during pipeline execution +RSpec.describe "Performance" do + describe "model without scope" do + context "on #update" do + subject do + create(:basic_model, name: "target").update!(position: 0) + end + + before do + attrs = (0...500_000).to_a.map do |position| + "('#{SecureRandom.alphanumeric(8)}', #{position})" + end + + BasicModel.connection.execute(<<-SQL) + INSERT INTO basic_models (name, position) + VALUES + #{attrs.join(', ')}; + SQL + end + + let(:offset) { rand(1..500) } + let!(:expected_result) do + BasicModel.ordered(:asc).offset(offset).first(10).pluck(:name, :position).to_h + .transform_values { |v| v + 1 } + end + + it "shifts the records" do + exec_time = Benchmark.measure { subject } + # TODO: Check object allocation + expect(exec_time.real).to be < 6.8 + expect(BasicModel.ordered(:asc).offset(offset + 1).first(10).pluck(:name, :position).to_h) + .to eq(expected_result) + end + end + end +end From 7f7b01de24a7bca1968e70204f22346f43cb1c6d Mon Sep 17 00:00:00 2001 From: Mateusz Lata Date: Wed, 28 Jun 2023 18:10:06 +0200 Subject: [PATCH 2/6] ODA: Cover with test example performance of basic model with and without index. --- spec/performance/index_spec.rb | 111 +++++++++++++++++++++++++++ spec/performance/no_index_spec.rb | 120 ++++++++++++++++++++++++++++++ spec/performance_spec.rb | 39 ---------- spec/spec_helper.rb | 13 ++++ 4 files changed, 244 insertions(+), 39 deletions(-) create mode 100644 spec/performance/index_spec.rb create mode 100644 spec/performance/no_index_spec.rb delete mode 100644 spec/performance_spec.rb diff --git a/spec/performance/index_spec.rb b/spec/performance/index_spec.rb new file mode 100644 index 0000000..ae07a1d --- /dev/null +++ b/spec/performance/index_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +RSpec.describe "Performance with index" do + before(:all) do + attrs = (0...100_000).to_a.map do |position| + "('#{SecureRandom.alphanumeric(8)}', #{position})" + end + + BasicModel.connection.execute(<<-SQL) + INSERT INTO basic_models (name, position) + VALUES + #{attrs.join(', ')}; + SQL + end + + after(:all) { BasicModel.delete_all } + + describe "model without scope" do + context "on #update" do + subject { create(:basic_model).update!(position: position) } + + context "with position set as first value" do + let(:position) { 0 } + + it "takes less than 1.25s" do + expect(elapsed_time { subject }).to be < 1.25 + end + end + + context "with position set as middle value" do + let(:position) { 50_000 } + + it "takes less than 0.75s" do + expect(elapsed_time { subject }).to be < 0.75 + end + end + + context "with position set as last value" do + let(:position) { 99_999 } + + it "takes less than 0.15s" do + expect(elapsed_time { subject }).to be < 0.15 + end + end + end + + context "on #create" do + subject { create(:basic_model, **attrs) } + let(:attrs) { {} } + + context "without position specified" do + it "takes less than 0.07s" do + expect(elapsed_time { subject }).to be < 0.07 + end + end + + context "with postion set as 3/4 of total count" do + let(:attrs) { { position: 75_000 } } + + it "takes less than 0.35s" do + expect(elapsed_time { subject }).to be < 0.35 + end + end + + context "with position set as the middle value" do + let(:attrs) { { position: 50_000 } } + + it "takes less than 0.8s" do + expect(elapsed_time { subject }).to be < 0.8 + end + end + + context "with position set as first value" do + let(:attrs) { { position: 0 } } + + it "takes less than 1.5s" do + expect(elapsed_time { subject }).to be < 1.5 + end + end + end + + context "on #destroy" do + subject { record.destroy! } + let!(:record) { create(:basic_model, **attrs) } + + context "with position set as last value" do + let(:attrs) { { position: 100_000 } } + + it "takes less than 0.01s" do + expect(elapsed_time { subject }).to be < 0.01 + end + end + + context "with position set as middle value" do + let(:attrs) { { position: 50_000 } } + + it "takes less than 0.85s" do + expect(elapsed_time { subject }).to be < 0.85 + end + end + + context "with position set as first value" do + let(:attrs) { { position: 0 } } + + it "takes less than 1.5s" do + expect(elapsed_time { subject }).to be < 1.5 + end + end + end + end +end diff --git a/spec/performance/no_index_spec.rb b/spec/performance/no_index_spec.rb new file mode 100644 index 0000000..2b04a64 --- /dev/null +++ b/spec/performance/no_index_spec.rb @@ -0,0 +1,120 @@ +# frozen_string_literal: true + +RSpec.describe "Performance with index" do + before(:all) do + attrs = (0...100_000).to_a.map do |position| + "('#{SecureRandom.alphanumeric(8)}', #{position})" + end + + BasicModel.connection.execute(<<-SQL) + ALTER TABLE basic_models + DROP CONSTRAINT basic_models_position_key; + + INSERT INTO basic_models (name, position) + VALUES + #{attrs.join(', ')}; + SQL + end + + after(:all) do + BasicModel.delete_all + BasicModel.connection.execute(<<-SQL) + ALTER TABLE basic_models + ADD UNIQUE(position) DEFERRABLE INITIALLY DEFERRED; + SQL + end + + describe "model without scope" do + context "on #update" do + subject { create(:basic_model).update!(position: position) } + + context "with position set as first value" do + let(:position) { 0 } + + it "takes less than 0.9s" do + expect(elapsed_time { subject }).to be < 0.9 + end + end + + context "with position set as middle value" do + let(:position) { 50_000 } + + it "takes less than 0.5s" do + expect(elapsed_time { subject }).to be < 0.5 + end + end + + context "with position set as last value" do + let(:position) { 99_999 } + + it "takes less than 0.09s" do + expect(elapsed_time { subject }).to be < 0.09 + end + end + end + + context "on #create" do + subject { create(:basic_model, **attrs) } + let(:attrs) { {} } + + context "without position specified" do + it "takes less than 0.04s" do + expect(elapsed_time { subject }).to be < 0.04 + end + end + + context "with postion set as 3/4 of total count" do + let(:attrs) { { position: 75_000 } } + + it "takes less than 0.25s" do + expect(elapsed_time { subject }).to be < 0.25 + end + end + + context "with position set as the middle value" do + let(:attrs) { { position: 50_000 } } + + it "takes less than 0.5s" do + expect(elapsed_time { subject }).to be < 0.5 + end + end + + context "with position set as first value" do + let(:attrs) { { position: 0 } } + + it "takes less than 0.9s" do + expect(elapsed_time { subject }).to be < 0.9 + end + end + end + + context "on #destroy" do + subject { record.destroy! } + let!(:record) { create(:basic_model, **attrs) } + + context "with position set as last value" do + let(:attrs) { { position: 100_000 } } + + it "takes less than 0.025s" do + expect(elapsed_time { subject }).to be < 0.025 + end + end + + context "with position set as middle value" do + let(:attrs) { { position: 50_000 } } + + it "takes less than 0.6s" do + expect(elapsed_time { subject }).to be < 0.6 + end + end + + context "with position set as first value" do + let(:attrs) { { position: 0 } } + + it "takes less than 1.15s" do + expect(elapsed_time { subject }).to be < 1.15 + end + end + end + end +end diff --git a/spec/performance_spec.rb b/spec/performance_spec.rb deleted file mode 100644 index d00ed1c..0000000 --- a/spec/performance_spec.rb +++ /dev/null @@ -1,39 +0,0 @@ -# frozen_string_literal: true - -require "benchmark" -# TODO: Turn off performance-related tests during pipeline execution -RSpec.describe "Performance" do - describe "model without scope" do - context "on #update" do - subject do - create(:basic_model, name: "target").update!(position: 0) - end - - before do - attrs = (0...500_000).to_a.map do |position| - "('#{SecureRandom.alphanumeric(8)}', #{position})" - end - - BasicModel.connection.execute(<<-SQL) - INSERT INTO basic_models (name, position) - VALUES - #{attrs.join(', ')}; - SQL - end - - let(:offset) { rand(1..500) } - let!(:expected_result) do - BasicModel.ordered(:asc).offset(offset).first(10).pluck(:name, :position).to_h - .transform_values { |v| v + 1 } - end - - it "shifts the records" do - exec_time = Benchmark.measure { subject } - # TODO: Check object allocation - expect(exec_time.real).to be < 6.8 - expect(BasicModel.ordered(:asc).offset(offset + 1).first(10).pluck(:name, :position).to_h) - .to eq(expected_result) - end - end - end -end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index c6f2e04..1ef44cf 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -7,6 +7,8 @@ require "shoulda-matchers" require "factory_bot_rails" require "ammeter/init" +require "rspec-benchmark" +require "benchmark" require_relative "./factories" Dir["./support/*.rb"].sort.each { |file| require file } @@ -48,6 +50,11 @@ config.include(Shoulda::Matchers::ActiveModel, :with_validations) config.include(Shoulda::Matchers::ActiveRecord, :with_validations) config.include FactoryBot::Syntax::Methods + config.include RSpec::Benchmark::Matchers +end + +RSpec::Benchmark.configure do |config| + config.run_in_subprocess = true end Shoulda::Matchers.configure do |config| @@ -57,3 +64,9 @@ with.library :active_model end end + +def elapsed_time(&block) + Benchmark.measure do + block.call + end.real +end From 22a2f84aee5c055a3148b82d74aa6a60be42ee45 Mon Sep 17 00:00:00 2001 From: Mateusz Lata Date: Wed, 28 Jun 2023 18:15:48 +0200 Subject: [PATCH 3/6] ODA: Exclude performance test on GH actions. Clean up spec helper. --- .github/workflows/coverage.yml | 2 +- .github/workflows/pull_request.yml | 2 +- spec/spec_helper.rb | 6 ------ 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 85a977f..a0aff70 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -43,7 +43,7 @@ jobs: bundler-cache: true - name: Run RSpec - run: bundle exec rspec -fd + run: bundle exec rspec -fd --exclude-pattern "spec/performance/*.rb" - name: Save code coverage id: coverage diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index f522fae..931fd12 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -70,4 +70,4 @@ jobs: bundler-cache: true - name: Run RSpec - run: bundle exec rspec -fd + run: bundle exec rspec -fd --exclude-pattern "spec/performance/*.rb" diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6c67eec..4d56708 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -8,7 +8,6 @@ require "shoulda-matchers" require "factory_bot_rails" require "ammeter/init" -require "rspec-benchmark" require "benchmark" require_relative "./factories" Dir["./support/*.rb"].sort.each { |file| require file } @@ -55,11 +54,6 @@ config.include(Shoulda::Matchers::ActiveModel, :with_validations) config.include(Shoulda::Matchers::ActiveRecord, :with_validations) config.include FactoryBot::Syntax::Methods - config.include RSpec::Benchmark::Matchers -end - -RSpec::Benchmark.configure do |config| - config.run_in_subprocess = true end Shoulda::Matchers.configure do |config| From aadfc1189fe10bd5ef3c9601de9c9f4401f9ff4d Mon Sep 17 00:00:00 2001 From: Mateusz Lata Date: Wed, 28 Jun 2023 18:21:02 +0200 Subject: [PATCH 4/6] ODA: Don not run GH actions on drafts --- .github/workflows/pull_request.yml | 1 + spec/spec_helper.rb | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 931fd12..59ed94b 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -1,6 +1,7 @@ name: Pull request workflow on: pull_request: + types: [review_requested, ready_for_review] branches: - master jobs: diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 4d56708..a644551 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -8,7 +8,6 @@ require "shoulda-matchers" require "factory_bot_rails" require "ammeter/init" -require "benchmark" require_relative "./factories" Dir["./support/*.rb"].sort.each { |file| require file } From f2d16cb315216caaa61e5111dd7ae8d73f056683 Mon Sep 17 00:00:00 2001 From: Mateusz Lata Date: Wed, 28 Jun 2023 18:29:09 +0200 Subject: [PATCH 5/6] ODA: fix typo --- README.md | 4 ++-- spec/performance/no_index_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 07819c3..1f44b14 100644 --- a/README.md +++ b/README.md @@ -163,7 +163,7 @@ photo.destroy() Photo.profile.ordered.pluck(:label, :position) # => [["A", 0], ["B", 1], ["C", 2]] Photo.cover.ordered.pluck(:label, :position) # => [["D", 0], ["E", 1]] ``` -#### Default push front +#### Auto set ```ruby class Image < ActiveRecord::Base @@ -216,7 +216,7 @@ Image.create(label: "B") Image.ordered.pluck(:label, :position) # => [["A", 10], ["B", 11]] ``` -### Decremental sequence +#### Decremental sequence ```ruby class Image < ActiveRecord::Base diff --git a/spec/performance/no_index_spec.rb b/spec/performance/no_index_spec.rb index 2b04a64..b0bbf5d 100644 --- a/spec/performance/no_index_spec.rb +++ b/spec/performance/no_index_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe "Performance with index" do +RSpec.describe "Performance no index" do before(:all) do attrs = (0...100_000).to_a.map do |position| "('#{SecureRandom.alphanumeric(8)}', #{position})" From 15d13aee917f3d59497eb9eb9a54a4eb9a63cdb8 Mon Sep 17 00:00:00 2001 From: Mateusz Lata Date: Thu, 29 Jun 2023 12:21:57 +0200 Subject: [PATCH 6/6] ODA: Implement suggestions after code review. --- .github/workflows/coverage.yml | 3 ++- .github/workflows/pull_request.yml | 4 ++-- spec/performance/index_spec.rb | 2 +- spec/performance/no_index_spec.rb | 2 +- spec/spec_helper.rb | 2 ++ 5 files changed, 8 insertions(+), 5 deletions(-) diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index a0aff70..a6361ac 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -33,6 +33,7 @@ jobs: POSTGRES_USERNAME: postgres POSTGRES_PASSWORD: postgres SCHEMA: structure.sql + PERFORMANCE_TESTS_DISABLED: 1 steps: - name: Checkout repository uses: actions/checkout@v3 @@ -43,7 +44,7 @@ jobs: bundler-cache: true - name: Run RSpec - run: bundle exec rspec -fd --exclude-pattern "spec/performance/*.rb" + run: bundle exec rspec -fd - name: Save code coverage id: coverage diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 59ed94b..6712f67 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -1,7 +1,6 @@ name: Pull request workflow on: pull_request: - types: [review_requested, ready_for_review] branches: - master jobs: @@ -59,6 +58,7 @@ jobs: POSTGRES_PASSWORD: postgres BUNDLE_GEMFILE: ${{ matrix.gemfile }} SPEC_DISABLE_DROP_DATABASE: 1 + PERFORMANCE_TESTS_DISABLED: 1 SCHEMA: structure.sql steps: - name: Checkout repository @@ -71,4 +71,4 @@ jobs: bundler-cache: true - name: Run RSpec - run: bundle exec rspec -fd --exclude-pattern "spec/performance/*.rb" + run: bundle exec rspec -fd diff --git a/spec/performance/index_spec.rb b/spec/performance/index_spec.rb index ae07a1d..e730c0d 100644 --- a/spec/performance/index_spec.rb +++ b/spec/performance/index_spec.rb @@ -2,7 +2,7 @@ RSpec.describe "Performance with index" do before(:all) do - attrs = (0...100_000).to_a.map do |position| + attrs = (0...100_000).map do |position| "('#{SecureRandom.alphanumeric(8)}', #{position})" end diff --git a/spec/performance/no_index_spec.rb b/spec/performance/no_index_spec.rb index b0bbf5d..c7c777f 100644 --- a/spec/performance/no_index_spec.rb +++ b/spec/performance/no_index_spec.rb @@ -2,7 +2,7 @@ RSpec.describe "Performance no index" do before(:all) do - attrs = (0...100_000).to_a.map do |position| + attrs = (0...100_000).map do |position| "('#{SecureRandom.alphanumeric(8)}', #{position})" end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index a644551..9c38a0d 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -36,6 +36,8 @@ end end + config.exclude_pattern = "spec/performance/*.rb" if ENV["PERFORMANCE_TESTS_DISABLED"] == "1" + config.before(:suite) do Rake::Task["db:create"].invoke Rake::Task["db:migrate"].invoke