Commit 5fc166b9 authored by Stan Hu's avatar Stan Hu

Cache CI expire_in parsing

Previously the CI processor spent a surprising amount of time
validating the `expire_in` field for each build in the pipeline. It
turns out `ChronicDuration.parse` can be expensive because it uses
regular expressions and ActiveSupport numeric conversions.

We observe that builds often use a small set of `expire_in` values, so
we can cache the result of a previous validation.

For a pipeline like `gitlab-org/gitlab` that has 200 builds with 8
different `expire_in` values, we see a 28x improvement:

```
Calculating -------------------------------------
                 old     1.000  i/100ms
                 new    14.000  i/100ms
-------------------------------------------------
                 old      5.275  (± 0.0%) i/s -     27.000
                 new    149.409  (± 5.4%) i/s -    756.000

Comparison:
                 new:      149.4 i/s
                 old:        5.3 i/s - 28.32x slower
```

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/322386

Changelog: performance
parent 94c78fd6
......@@ -16,9 +16,7 @@ module Gitlab
def validate_duration
return true if never?
parse
rescue ChronicDuration::DurationParseError
false
cached_parse
end
def seconds_from_now
......@@ -29,12 +27,28 @@ module Gitlab
attr_reader :value
def cached_parse
return validation_cache[value] if validation_cache.key?(value)
validation_cache[value] = safe_parse
end
def safe_parse
parse
rescue ChronicDuration::DurationParseError
false
end
def parse
return if never?
ChronicDuration.parse(value)
end
def validation_cache
Gitlab::SafeRequestStore[:ci_expire_in_parser_cache] ||= {}
end
def never?
value.to_s.casecmp('never') == 0
end
......
......@@ -3,7 +3,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Ci::Build::Artifacts::ExpireInParser do
describe '.validate_duration' do
describe '.validate_duration', :request_store do
subject { described_class.validate_duration(value) }
context 'with never' do
......@@ -20,14 +20,33 @@ RSpec.describe Gitlab::Ci::Build::Artifacts::ExpireInParser do
context 'with a duration' do
let(:value) { '1 Day' }
let(:other_value) { '30 seconds' }
it { is_expected.to be_truthy }
it 'caches data' do
expect(ChronicDuration).to receive(:parse).with(value).once.and_call_original
expect(ChronicDuration).to receive(:parse).with(other_value).once.and_call_original
2.times do
expect(described_class.validate_duration(value)).to eq(86400)
expect(described_class.validate_duration(other_value)).to eq(30)
end
end
end
context 'without a duration' do
let(:value) { 'something' }
it { is_expected.to be_falsy }
it 'caches data' do
expect(ChronicDuration).to receive(:parse).with(value).once.and_call_original
2.times do
expect(described_class.validate_duration(value)).to be_falsey
end
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment