Reduce variables transformations when creating a new pipeline

Changelog: performance

lib/gitlab/ci/build/context/base.rb

@@ -17,6 +17,12 @@ module Gitlab
             raise NotImplementedError
           end
 
+          def variables_hash
+            strong_memoize(:variables_hash) do
+              variables.to_hash
+            end
+          end
+
           def project
             pipeline.project
           end

lib/gitlab/ci/build/policy/variables.rb

@@ -10,7 +10,7 @@ module Gitlab
           end
 
           def satisfied_by?(pipeline, context)
-            variables = context.variables
+            variables = context.variables_hash
 
             statements = @expressions.map do |statement|
               ::Gitlab::Ci::Pipeline::Expression::Statement

lib/gitlab/ci/build/rules/rule/clause/changes.rb

@@ -23,7 +23,7 @@ module Gitlab
           return @globs unless context
 
           @globs.map do |glob|
-            ExpandVariables.expand_existing(glob, context.variables)
+            ExpandVariables.expand_existing(glob, -> { context.variables_hash })
           end
         end
       end

lib/gitlab/ci/build/rules/rule/clause/if.rb

@@ -10,7 +10,7 @@ module Gitlab
 
         def satisfied_by?(pipeline, context)
           ::Gitlab::Ci::Pipeline::Expression::Statement.new(
-            @expression, context.variables).truthful?
+            @expression, context.variables_hash).truthful?
         end
       end
     end

lib/gitlab/ci/config/external/context.rb

@@ -9,17 +9,19 @@ module Gitlab
 
           TimeoutError = Class.new(StandardError)
 
+          include ::Gitlab::Utils::StrongMemoize
+
           attr_reader :project, :sha, :user, :parent_pipeline, :variables
           attr_reader :expandset, :execution_deadline, :logger
 
           delegate :instrument, to: :logger
 
-          def initialize(project: nil, sha: nil, user: nil, parent_pipeline: nil, variables: [], logger: nil)
+          def initialize(project: nil, sha: nil, user: nil, parent_pipeline: nil, variables: nil, logger: nil)
             @project = project
             @sha = sha
             @user = user
             @parent_pipeline = parent_pipeline
-            @variables = variables
+            @variables = variables || Ci::Variables::Collection.new
             @expandset = Set.new
             @execution_deadline = 0
             @logger = logger || Gitlab::Ci::Pipeline::Logger.new(project: project)
@@ -39,6 +41,12 @@ module Gitlab
             end
           end
 
+          def variables_hash
+            strong_memoize(:variables_hash) do
+              variables.to_hash
+            end
+          end
+
           def mutate(attrs = {})
             self.class.new(**attrs) do |ctx|
               ctx.expandset = expandset

lib/gitlab/ci/config/external/mapper.rb

@@ -179,7 +179,7 @@ module Gitlab
           end
 
           def expand(data)
-            ExpandVariables.expand(data, context.variables)
+            ExpandVariables.expand(data, -> { context.variables_hash })
           end
         end
       end

lib/gitlab/ci/pipeline/expression/lexeme/variable.rb

@@ -9,7 +9,11 @@ module Gitlab
             PATTERN = /\$(?<name>\w+)/.freeze
 
             def evaluate(variables = {})
-              variables.with_indifferent_access.fetch(@value, nil)
+              unless variables.is_a?(ActiveSupport::HashWithIndifferentAccess)
+                variables = variables.with_indifferent_access
+              end
+
+              variables.fetch(@value, nil)
             end
 
             def inspect

lib/gitlab/ci/pipeline/expression/statement.rb

@@ -9,7 +9,7 @@ module Gitlab
 
           def initialize(statement, variables = nil)
             @lexer = Expression::Lexer.new(statement)
-            @variables = variables&.to_hash
+            @variables = variables || {}
           end
 
           def parse_tree
@@ -19,7 +19,7 @@ module Gitlab
           end
 
           def evaluate
-            parse_tree.evaluate(@variables.to_h)
+            parse_tree.evaluate(@variables)
           end
 
           def truthful?

lib/gitlab/ci/pipeline/seed/build.rb

@@ -205,7 +205,7 @@ module Gitlab
 
           def evaluate_runner_tags
             @seed_attributes[:tag_list]&.map do |tag|
-              ExpandVariables.expand_existing(tag, evaluate_context.variables)
+              ExpandVariables.expand_existing(tag, -> { evaluate_context.variables_hash })
             end
           end
 

spec/lib/gitlab/ci/build/context/build_spec.rb

@@ -8,11 +8,7 @@ RSpec.describe Gitlab::Ci::Build::Context::Build do
 
   let(:context) { described_class.new(pipeline, seed_attributes) }
 
-  describe '#variables' do
-    subject { context.variables.to_hash }
-
-    it { expect(context.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) }
-
+  shared_examples 'variables collection' do
     it { is_expected.to include('CI_COMMIT_REF_NAME' => 'master') }
     it { is_expected.to include('CI_PIPELINE_IID'    => pipeline.iid.to_s) }
     it { is_expected.to include('CI_PROJECT_PATH'    => pipeline.project.full_path) }
@@ -27,4 +23,20 @@ RSpec.describe Gitlab::Ci::Build::Context::Build do
       it { is_expected.to include('CI_PROJECT_PATH'   => pipeline.project.full_path) }
     end
   end
+
+  describe '#variables' do
+    subject { context.variables.to_hash }
+
+    it { expect(context.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) }
+
+    it_behaves_like 'variables collection'
+  end
+
+  describe '#variables_hash' do
+    subject { context.variables_hash }
+
+    it { expect(context.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) }
+
+    it_behaves_like 'variables collection'
+  end
 end

spec/lib/gitlab/ci/build/context/global_spec.rb

@@ -8,11 +8,7 @@ RSpec.describe Gitlab::Ci::Build::Context::Global do
 
   let(:context) { described_class.new(pipeline, yaml_variables: yaml_variables) }
 
-  describe '#variables' do
-    subject { context.variables.to_hash }
-
-    it { expect(context.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) }
-
+  shared_examples 'variables collection' do
     it { is_expected.to include('CI_COMMIT_REF_NAME' => 'master') }
     it { is_expected.to include('CI_PIPELINE_IID'    => pipeline.iid.to_s) }
     it { is_expected.to include('CI_PROJECT_PATH'    => pipeline.project.full_path) }
@@ -26,4 +22,20 @@ RSpec.describe Gitlab::Ci::Build::Context::Global do
       it { is_expected.to include('SUPPORTED' => 'parsed') }
     end
   end
+
+  describe '#variables' do
+    subject { context.variables.to_hash }
+
+    it { expect(context.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) }
+
+    it_behaves_like 'variables collection'
+  end
+
+  describe '#variables_hash' do
+    subject { context.variables_hash }
+
+    it { is_expected.to be_instance_of(ActiveSupport::HashWithIndifferentAccess) }
+
+    it_behaves_like 'variables collection'
+  end
 end

spec/lib/gitlab/ci/build/policy/variables_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Gitlab::Ci::Build::Policy::Variables do
   let(:seed) do
     double('build seed',
       to_resource: ci_build,
-      variables: ci_build.scoped_variables
+      variables_hash: ci_build.scoped_variables.to_hash
     )
   end
 
@@ -91,7 +91,7 @@ RSpec.describe Gitlab::Ci::Build::Policy::Variables do
       let(:seed) do
         double('bridge seed',
           to_resource: bridge,
-          variables: ci_build.scoped_variables
+          variables_hash: ci_build.scoped_variables.to_hash
         )
       end
 

spec/lib/gitlab/ci/build/rules/rule/clause/changes_spec.rb

@@ -33,12 +33,12 @@ RSpec.describe Gitlab::Ci::Build::Rules::Rule::Clause::Changes do
       end
 
       context 'when context has the specified variables' do
-        let(:variables) do
-          [{ key: "HELM_DIR", value: "helm", public: true }]
+        let(:variables_hash) do
+          { 'HELM_DIR' => 'helm' }
         end
 
         before do
-          allow(context).to receive(:variables).and_return(variables)
+          allow(context).to receive(:variables_hash).and_return(variables_hash)
         end
 
         it { is_expected.to be_truthy }
@@ -49,7 +49,7 @@ RSpec.describe Gitlab::Ci::Build::Rules::Rule::Clause::Changes do
         let(:modified_paths) { ['path/with/$in/it/file.txt'] }
 
         before do
-          allow(context).to receive(:variables).and_return([])
+          allow(context).to receive(:variables_hash).and_return({})
         end
 
         it { is_expected.to be_truthy }

spec/lib/gitlab/ci/build/rules/rule_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Gitlab::Ci::Build::Rules::Rule do
   let(:seed) do
     double('build seed',
       to_resource: ci_build,
-      variables: ci_build.scoped_variables
+      variables_hash: ci_build.scoped_variables.to_hash
     )
   end
 

spec/lib/gitlab/ci/build/rules_spec.rb

@@ -3,13 +3,13 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Build::Rules do
-  let(:pipeline) { create(:ci_pipeline) }
-  let(:ci_build) { build(:ci_build, pipeline: pipeline) }
+  let_it_be(:pipeline) { create(:ci_pipeline) }
+  let_it_be(:ci_build) { build(:ci_build, pipeline: pipeline) }
 
   let(:seed) do
     double('build seed',
       to_resource: ci_build,
-      variables: ci_build.scoped_variables
+      variables_hash: ci_build.scoped_variables.to_hash
     )
   end
 

spec/lib/gitlab/ci/config/external/context_spec.rb

@@ -6,7 +6,8 @@ RSpec.describe Gitlab::Ci::Config::External::Context do
   let(:project) { double('Project') }
   let(:user) { double('User') }
   let(:sha) { '12345' }
-  let(:attributes) { { project: project, user: user, sha: sha } }
+  let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'a', 'value' => 'b' }]) }
+  let(:attributes) { { project: project, user: user, sha: sha, variables: variables } }
 
   subject(:subject) { described_class.new(**attributes) }
 
@@ -15,6 +16,9 @@ RSpec.describe Gitlab::Ci::Config::External::Context do
       it { is_expected.to have_attributes(**attributes) }
       it { expect(subject.expandset).to eq(Set.new) }
       it { expect(subject.execution_deadline).to eq(0) }
+      it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) }
+      it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) }
+      it { expect(subject.variables_hash).to include('a' => 'b') }
     end
 
     context 'without values' do
@@ -23,6 +27,8 @@ RSpec.describe Gitlab::Ci::Config::External::Context do
       it { is_expected.to have_attributes(**attributes) }
       it { expect(subject.expandset).to eq(Set.new) }
       it { expect(subject.execution_deadline).to eq(0) }
+      it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) }
+      it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) }
     end
   end
 

spec/lib/gitlab/ci/config/external/rules_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Gitlab::Ci::Config::External::Rules do
   subject(:rules) { described_class.new(rule_hashes) }
 
   describe '#evaluate' do
-    let(:context) { double(variables: {}) }
+    let(:context) { double(variables_hash: {}) }
 
     subject(:result) { rules.evaluate(context).pass? }
 
@@ -20,13 +20,13 @@ RSpec.describe Gitlab::Ci::Config::External::Rules do
       let(:rule_hashes) { [{ if: '$MY_VAR == "hello"' }] }
 
       context 'when the rule matches' do
-        let(:context) { double(variables: { MY_VAR: 'hello' }) }
+        let(:context) { double(variables_hash: { 'MY_VAR' => 'hello' }) }
 
         it { is_expected.to eq(true) }
       end
 
       context 'when the rule does not match' do
-        let(:context) { double(variables: { MY_VAR: 'invalid' }) }
+        let(:context) { double(variables_hash: { 'MY_VAR' => 'invalid' }) }
 
         it { is_expected.to eq(false) }
       end

spec/lib/gitlab/ci/pipeline/expression/lexeme/variable_spec.rb

@@ -17,30 +17,33 @@ RSpec.describe Gitlab::Ci::Pipeline::Expression::Lexeme::Variable do
   end
 
   describe '#evaluate' do
-    it 'returns variable value if it is defined' do
-      variable = described_class.new('VARIABLE')
+    let(:lexeme) { described_class.new('VARIABLE') }
 
-      expect(variable.evaluate(VARIABLE: 'my variable'))
+    it 'returns variable value if it is defined' do
+      expect(lexeme.evaluate(VARIABLE: 'my variable'))
         .to eq 'my variable'
     end
 
     it 'allows to use a string as a variable key too' do
-      variable = described_class.new('VARIABLE')
-
-      expect(variable.evaluate('VARIABLE' => 'my variable'))
+      expect(lexeme.evaluate('VARIABLE' => 'my variable'))
         .to eq 'my variable'
     end
 
     it 'returns nil if it is not defined' do
-      variable = described_class.new('VARIABLE')
-
-      expect(variable.evaluate(OTHER: 'variable')).to be_nil
+      expect(lexeme.evaluate('OTHER' => 'variable')).to be_nil
+      expect(lexeme.evaluate(OTHER: 'variable')).to be_nil
     end
 
     it 'returns an empty string if it is empty' do
-      variable = described_class.new('VARIABLE')
+      expect(lexeme.evaluate('VARIABLE' => '')).to eq ''
+      expect(lexeme.evaluate(VARIABLE: '')).to eq ''
+    end
+
+    it 'does not call with_indifferent_access unnecessarily' do
+      variables_hash = { VARIABLE: 'my variable' }.with_indifferent_access
 
-      expect(variable.evaluate(VARIABLE: '')).to eq ''
+      expect(variables_hash).not_to receive(:with_indifferent_access)
+      expect(lexeme.evaluate(variables_hash)).to eq 'my variable'
     end
   end
 end

spec/lib/gitlab/ci/pipeline/expression/statement_spec.rb

@@ -9,6 +9,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Expression::Statement do
       .append(key: 'PATH_VARIABLE', value: 'a/path/variable/value')
       .append(key: 'FULL_PATH_VARIABLE', value: '/a/full/path/variable/value')
       .append(key: 'EMPTY_VARIABLE', value: '')
+      .to_hash
   end
 
   subject do