From 3cbf65c3e85903cd2b928dc2ed5efac04ad41aa2 Mon Sep 17 00:00:00 2001 From: Daniel Gollahon Date: Sun, 14 May 2023 11:35:51 -0700 Subject: [PATCH 1/2] Remove redundant skipped candidate specs - An identical expectation is included via the `skipped candidate` shared example in `spec/shared/method_matcher_behavior.rb`. --- .../mutant/matcher/method/instance_spec.rb | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/spec/unit/mutant/matcher/method/instance_spec.rb b/spec/unit/mutant/matcher/method/instance_spec.rb index 212537ba5..b3082e045 100644 --- a/spec/unit/mutant/matcher/method/instance_spec.rb +++ b/spec/unit/mutant/matcher/method/instance_spec.rb @@ -56,10 +56,6 @@ def arguments end include_examples 'skipped candidate' - - it 'returns expected subjects' do - expect(subject).to eql([]) - end end context 'when method is defined inside eval' do @@ -73,10 +69,6 @@ def arguments end include_examples 'skipped candidate' - - it 'returns expected subjects' do - expect(subject).to eql([]) - end end context 'when method is defined without source location' do @@ -90,10 +82,6 @@ def arguments end include_examples 'skipped candidate' - - it 'returns expected subjects' do - expect(subject).to eql([]) - end end context 'in module eval' do @@ -106,10 +94,6 @@ def arguments end include_examples 'skipped candidate' - - it 'returns expected subjects' do - expect(subject).to eql([]) - end end context 'in class eval' do @@ -122,10 +106,6 @@ def arguments end include_examples 'skipped candidate' - - it 'returns expected subjects' do - expect(subject).to eql([]) - end end context 'when method is defined once' do From d115fc613cd4f56c36fa06bcb01a906eaa89f301 Mon Sep 17 00:00:00 2001 From: Daniel Gollahon Date: Sun, 14 May 2023 10:30:40 -0700 Subject: [PATCH 2/2] Change to more efficient subject matching in presence of repository diffs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Avoids parsing source files that live outside the specified diff. * Massively speeds up boot time on larger codebases and should be noticeable on all sizes. On `mutant` itself, when making a relatively targeted change (such as this one), boot time is reduced by about 0.7 seconds (~20% faster). On a larger project I work on, the speed up was 24 seconds or 2.5x faster overall! I left the original, full, subject filtering in place to simplify this change. This just short-circuits when the files we are interested in are not modified and `--since` is specified. * `mutant` run before on this change: ``` Benchmark 1: bundle exec mutant run --zombie --since HEAD~1 --profile Time (mean ± σ): 4.192 s ± 0.350 s [User: 8.567 s, System: 6.081 s] Range (min … max): 3.850 s … 4.960 s 10 runs ``` * `mutant` run after on this change: ``` Benchmark 1: bundle exec mutant run --zombie --since HEAD~1 --profile Time (mean ± σ): 3.535 s ± 0.213 s [User: 6.890 s, System: 5.784 s] Range (min … max): 3.188 s … 3.784 s 10 runs ``` * Also reduces memory consumption significantly since the ASTs for all source files are never constructed. In my larger project on very small single subject change it reduced memory allocations by 69MB (80% reduction) and reduced "retained" memory by 25MB (99.4% reduction). --- Changelog.md | 4 + lib/mutant/cli/command/environment.rb | 6 +- lib/mutant/matcher.rb | 4 +- lib/mutant/matcher/config.rb | 6 +- lib/mutant/matcher/method.rb | 9 +++ lib/mutant/repository.rb | 14 ---- spec/unit/mutant/cli_spec.rb | 30 ++----- spec/unit/mutant/matcher/config_spec.rb | 21 ++--- .../mutant/matcher/method/instance_spec.rb | 60 +++++++++++++- spec/unit/mutant/matcher_spec.rb | 79 ++++++++++++++----- .../mutant/repository/subject_filter_spec.rb | 30 ------- 11 files changed, 152 insertions(+), 111 deletions(-) delete mode 100644 spec/unit/mutant/repository/subject_filter_spec.rb diff --git a/Changelog.md b/Changelog.md index 670235ab1..4cf9edccd 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,9 @@ # v0.11.20 unreleased +* [#1382](https://github.com/mbj/mutant/pull/1382) + + Significantly optimize parsing overhead during boot. Smaller projects may see boot time speedups of 10-20%. Larger projects may see boot time improvements of 2-3x or greater. Memory consumption is also greatly reduced (by a factor of 5x or more in some small changesets). + * [#1378](https://github.com/mbj/mutant/pull/1379) Add support for user defined mutation worker process hooks. diff --git a/lib/mutant/cli/command/environment.rb b/lib/mutant/cli/command/environment.rb index 096b41b26..581571e9c 100644 --- a/lib/mutant/cli/command/environment.rb +++ b/lib/mutant/cli/command/environment.rb @@ -92,7 +92,6 @@ def add_integration_options(parser) end end - # rubocop:disable Metrics/MethodLength def add_matcher_options(parser) parser.separator('Matcher:') @@ -103,10 +102,7 @@ def add_matcher_options(parser) add_matcher(:start_expressions, @config.expression_parser.call(pattern).from_right) end parser.on('--since REVISION', 'Only select subjects touched since REVISION') do |revision| - add_matcher( - :subject_filters, - Repository::SubjectFilter.new(diff: Repository::Diff.new(to: revision, world: world)) - ) + add_matcher(:diffs, Repository::Diff.new(to: revision, world: world)) end end diff --git a/lib/mutant/matcher.rb b/lib/mutant/matcher.rb index 68fe1e978..7f58cce0f 100644 --- a/lib/mutant/matcher.rb +++ b/lib/mutant/matcher.rb @@ -31,7 +31,9 @@ def self.allowed_subject?(config, subject) private_class_method :allowed_subject? def self.select_subject?(config, subject) - config.subject_filters.all? { |filter| filter.call(subject) } + config.diffs.all? do |diff| + diff.touches?(subject.source_path, subject.source_lines) + end end private_class_method :select_subject? diff --git a/lib/mutant/matcher/config.rb b/lib/mutant/matcher/config.rb index e95b2c3f0..f6a04d555 100644 --- a/lib/mutant/matcher/config.rb +++ b/lib/mutant/matcher/config.rb @@ -8,7 +8,7 @@ class Config :ignore, :subjects, :start_expressions, - :subject_filters + :diffs ) INSPECT_FORMAT = "#<#{self} %s>" @@ -19,8 +19,8 @@ class Config PRESENTATIONS = { ignore: :syntax, start_expressions: :syntax, - subject_filters: :inspect, - subjects: :syntax + subjects: :syntax, + diffs: :inspect }.freeze private_constant(*constants(false)) diff --git a/lib/mutant/matcher/method.rb b/lib/mutant/matcher/method.rb index 05bb3141f..bbc54bf3b 100644 --- a/lib/mutant/matcher/method.rb +++ b/lib/mutant/matcher/method.rb @@ -120,6 +120,11 @@ def subject_config(node) def matched_view return if source_location.nil? + # This is a performance optimization when using --since to avoid the cost of parsing + # every source file that could possibly map to a subject. A more fine-grained filtering + # takes places later in the process. + return unless relevant_source_file? + ast .on_line(source_line) .select { |view| view.node.type.eql?(self.class::MATCH_NODE_TYPE) && match?(view.node) } @@ -127,6 +132,10 @@ def matched_view end memoize :matched_view + def relevant_source_file? + env.config.matcher.diffs.all? { |diff| diff.touches_path?(source_path) } + end + def visibility # This can be cleaned up once we are on >ruby-3.0 # Method#{public,private,protected}? exists there. diff --git a/lib/mutant/repository.rb b/lib/mutant/repository.rb index aca1e65aa..fd13dfc2e 100644 --- a/lib/mutant/repository.rb +++ b/lib/mutant/repository.rb @@ -2,19 +2,5 @@ module Mutant module Repository - # Subject filter based on repository diff - class SubjectFilter - include Adamantium, Anima.new(:diff) - - # Test if subject was touched in diff - # - # @param [Subject] subject - # - # @return [Boolean] - def call(subject) - diff.touches?(subject.source_path, subject.source_lines) - end - - end # SubjectFilter end # Repository end # Mutant diff --git a/spec/unit/mutant/cli_spec.rb b/spec/unit/mutant/cli_spec.rb index 5a140dd6c..a727ab646 100644 --- a/spec/unit/mutant/cli_spec.rb +++ b/spec/unit/mutant/cli_spec.rb @@ -1298,33 +1298,15 @@ def self.main_body let(:arguments) { super() + ['--since', 'reference'] } let(:load_config_file_config) do - super().with( - matcher: file_config.matcher.with( - subject_filters: [ - Mutant::Repository::SubjectFilter.new( - diff: Mutant::Repository::Diff.new( - to: 'reference', - world: world - ) - ) - ] - ) - ) + diff = Mutant::Repository::Diff.new(to: 'reference', world: world) + + super().with(matcher: file_config.matcher.with(diffs: [diff])) end let(:bootstrap_config) do - super().with( - matcher: file_config.matcher.with( - subject_filters: [ - Mutant::Repository::SubjectFilter.new( - diff: Mutant::Repository::Diff.new( - to: 'reference', - world: world - ) - ) - ] - ) - ) + diff = Mutant::Repository::Diff.new(to: 'reference', world: world) + + super().with(matcher: file_config.matcher.with(diffs: [diff])) end include_examples 'CLI run' diff --git a/spec/unit/mutant/matcher/config_spec.rb b/spec/unit/mutant/matcher/config_spec.rb index 56a0f1556..8966d42f3 100644 --- a/spec/unit/mutant/matcher/config_spec.rb +++ b/spec/unit/mutant/matcher/config_spec.rb @@ -13,8 +13,8 @@ def apply described_class.new( ignore: [parse_expression('Ignore#a')], start_expressions: [parse_expression('Start#a')], - subject_filters: [proc_a], - subjects: [parse_expression('Match#a')] + subjects: [parse_expression('Match#a')], + diffs: [] ) end @@ -22,8 +22,8 @@ def apply described_class.new( ignore: [parse_expression('Ignore#b')], start_expressions: [parse_expression('Start#b')], - subject_filters: [proc_b], - subjects: [parse_expression('Match#b')] + subjects: [parse_expression('Match#b')], + diffs: [] ) end @@ -32,8 +32,8 @@ def apply described_class.new( ignore: [parse_expression('Ignore#a'), parse_expression('Ignore#b')], start_expressions: [parse_expression('Start#a'), parse_expression('Start#b')], - subject_filters: [proc_a, proc_b], - subjects: [parse_expression('Match#a'), parse_expression('Match#b')] + subjects: [parse_expression('Match#a'), parse_expression('Match#b')], + diffs: [] ) ) end @@ -72,14 +72,5 @@ def apply it { should eql('#') } end - - context 'with subject filter' do - let(:object) do - described_class::DEFAULT - .add(:subject_filters, 'foo') - end - - it { should eql('#') } - end end end diff --git a/spec/unit/mutant/matcher/method/instance_spec.rb b/spec/unit/mutant/matcher/method/instance_spec.rb index b3082e045..79044410f 100644 --- a/spec/unit/mutant/matcher/method/instance_spec.rb +++ b/spec/unit/mutant/matcher/method/instance_spec.rb @@ -10,6 +10,7 @@ let(:object) { described_class.new(scope: scope, target_method: method) } let(:source_path) { MutantSpec::ROOT.join('test_app/lib/test_app.rb') } let(:type) { :def } + let(:config) { Mutant::Config::DEFAULT } let(:world) do instance_double( @@ -21,7 +22,7 @@ let(:env) do instance_double( Mutant::Env, - config: Mutant::Config::DEFAULT, + config: config, parser: Fixtures::TEST_ENV.parser, world: world ) @@ -58,6 +59,63 @@ def arguments include_examples 'skipped candidate' end + context 'when method is defined inside of a file removed by a diff filter' do + let(:config) { super().with(matcher: super().matcher.with(diffs: diffs)) } + let(:diff_a) { instance_double(Mutant::Repository::Diff, :diff_a) } + let(:diff_a_touches?) { false } + let(:diff_b) { instance_double(Mutant::Repository::Diff, :diff_b) } + let(:diff_b_touches?) { false } + let(:diffs) { [] } + let(:expected_warnings) { [] } + let(:method_line) { 13 } + let(:method_name) { :bar } + let(:scope) { base::WithMemoizer } + + before do + allow(diff_a).to receive(:touches_path?).with(source_path).and_return(diff_a_touches?) + allow(diff_b).to receive(:touches_path?).with(source_path).and_return(diff_b_touches?) + end + + context 'on absent diff filter' do + include_examples 'a method matcher' + end + + context 'on single diff filter' do + let(:diffs) { [diff_a] } + + context 'on not touching that diff' do + include_examples 'skipped candidate' + end + + context 'on touching that diff' do + let(:diff_a_touches?) { true } + + include_examples 'a method matcher' + end + end + + context 'on multiple diff filter' do + let(:diffs) { [diff_a, diff_b] } + + context 'on not touching any of the diffs' do + include_examples 'skipped candidate' + end + + context 'on touching one diff' do + let(:diff_a_touches?) { true } + + include_examples 'skipped candidate' + end + + context 'on touching both diffs' do + let(:diff_a_touches?) { true } + let(:diff_b_touches?) { true } + + include_examples 'a method matcher' + end + end + end + context 'when method is defined inside eval' do let(:scope) { base::WithMemoizer } let(:method) { scope.instance_method(:boz) } diff --git a/spec/unit/mutant/matcher_spec.rb b/spec/unit/mutant/matcher_spec.rb index ed153159b..ded903c43 100644 --- a/spec/unit/mutant/matcher_spec.rb +++ b/spec/unit/mutant/matcher_spec.rb @@ -7,11 +7,11 @@ def apply end let(:anon_matcher) { instance_double(Mutant::Matcher) } + let(:diffs) { [] } let(:env) { instance_double(Mutant::Env) } - let(:ignore_expressions) { [] } let(:expression_a) { expression('Foo::Bar#a', matcher_a) } let(:expression_b) { expression('Foo::Bar#b', matcher_b) } - let(:subject_filters) { [] } + let(:ignore_expressions) { [] } let(:matcher_a) do instance_double(Mutant::Matcher, call: [subject_a]) @@ -40,7 +40,7 @@ def apply ignore: ignore_expressions, subjects: [expression_a, expression_b], start_expressions: [], - subject_filters: subject_filters + diffs: diffs ) end @@ -49,7 +49,9 @@ def apply Mutant::Subject, 'subject a', expression: expression('Foo::Bar#a'), - inline_disabled?: false + inline_disabled?: false, + source_lines: 1..10, + source_path: 'subject/a.rb' ) end @@ -58,7 +60,9 @@ def apply Mutant::Subject, 'subject b', expression: expression('Foo::Bar#b'), - inline_disabled?: false + inline_disabled?: false, + source_lines: 100..101, + source_path: 'subject/b.rb' ) end @@ -98,25 +102,64 @@ def expression(input, matcher = anon_matcher) end end - context 'with subject filter' do - let(:subject_filters) do - [subject_a.public_method(:eql?)] - end + context 'with diffs' do + let(:diff_a) { instance_double(Mutant::Repository::Diff) } + let(:diff_b) { instance_double(Mutant::Repository::Diff) } - it 'returns expected subjects' do - expect(apply.call(env)).to eql([subject_a]) + let(:subject_a_touches_diff_a?) { false } + let(:subject_b_touches_diff_a?) { false } + let(:subject_a_touches_diff_b?) { false } + let(:subject_b_touches_diff_b?) { false } + + before do + allow(diff_a).to receive(:touches?) + .with(subject_a.source_path, subject_a.source_lines) + .and_return(subject_a_touches_diff_a?) + + allow(diff_a).to receive(:touches?) + .with(subject_b.source_path, subject_b.source_lines) + .and_return(subject_b_touches_diff_a?) + + allow(diff_b).to receive(:touches?) + .with(subject_a.source_path, subject_a.source_lines) + .and_return(subject_a_touches_diff_b?) + + allow(diff_b).to receive(:touches?) + .with(subject_b.source_path, subject_b.source_lines) + .and_return(subject_b_touches_diff_b?) end - end - context 'with subject ignore and filter' do - let(:ignore_expressions) { [subject_b.expression] } + context 'with one diff' do + let(:diffs) { [diff_a] } + + context 'when its touched by subject a' do + let(:subject_a_touches_diff_a?) { true } - let(:subject_filters) do - [subject_b.public_method(:eql?)] + it 'returns expected subjects' do + expect(apply.call(env)).to eql([subject_a]) + end + end end - it 'returns expected subjects' do - expect(apply.call(env)).to eql([]) + context 'with two diffs' do + let(:diffs) { [diff_a, diff_b] } + + context 'when one is touched by subject a' do + let(:subject_a_touches_diff_b?) { true } + + it 'returns expected subjects' do + expect(apply.call(env)).to eql([]) + end + end + + context 'when both are touched by subject a' do + let(:subject_a_touches_diff_a?) { true } + let(:subject_a_touches_diff_b?) { true } + + it 'returns expected subjects' do + expect(apply.call(env)).to eql([subject_a]) + end + end end end end diff --git a/spec/unit/mutant/repository/subject_filter_spec.rb b/spec/unit/mutant/repository/subject_filter_spec.rb deleted file mode 100644 index 29adbc08d..000000000 --- a/spec/unit/mutant/repository/subject_filter_spec.rb +++ /dev/null @@ -1,30 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe Mutant::Repository::SubjectFilter do - context '#call' do - subject { object.call(mutant_subject) } - - let(:object) { described_class.new(diff: diff) } - let(:diff) { instance_double(Mutant::Repository::Diff) } - let(:value) { instance_double(Object, 'value') } - - let(:mutant_subject) do - double( - 'Subject', - source_path: double('source path'), - source_lines: double('source lines') - ) - end - - before do - expect(diff).to receive(:touches?).with( - mutant_subject.source_path, - mutant_subject.source_lines - ).and_return(value) - end - - it 'connects return value to repository diff API' do - expect(subject).to be(value) - end - end -end