Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Significantly optimize parsing overhead when --since is specified #1382

Merged
merged 2 commits into from
May 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
6 changes: 1 addition & 5 deletions lib/mutant/cli/command/environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ def add_integration_options(parser)
end
end

# rubocop:disable Metrics/MethodLength
def add_matcher_options(parser)
parser.separator('Matcher:')

Expand All @@ -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,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgollahon I could get a rid of the entire subject_filters after checking private integrations, they where ported to the hooks subsystem so no need to maintain this API. this overall makes this commit much nicer than previous versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you for validating and simplifying that. I think it's much nicer code-wise, just didn't realize it was an option.

Repository::SubjectFilter.new(diff: Repository::Diff.new(to: revision, world: world))
)
add_matcher(:diffs, Repository::Diff.new(to: revision, world: world))
end
end

Expand Down
4 changes: 3 additions & 1 deletion lib/mutant/matcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Expand Down
6 changes: 3 additions & 3 deletions lib/mutant/matcher/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class Config
:ignore,
:subjects,
:start_expressions,
:subject_filters
:diffs
)

INSPECT_FORMAT = "#<#{self} %s>"
Expand All @@ -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))
Expand Down
9 changes: 9 additions & 0 deletions lib/mutant/matcher/method.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,22 @@ def subject_config(node)
def matched_view
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbj there is something wrong or incomplete with mutant or the zombification process. When I tried to mutate Mutant::Matcher::Method it found zero subjects. I had the same behavior on master as well so it was not introduced by this change.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works perfectly for me on main. Can you share a reproducing CLI?

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) }
.last
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.
Expand Down
14 changes: 0 additions & 14 deletions lib/mutant/repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
30 changes: 6 additions & 24 deletions spec/unit/mutant/cli_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
21 changes: 6 additions & 15 deletions spec/unit/mutant/matcher/config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,17 @@ 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

let(:other) do
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

Expand All @@ -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
Expand Down Expand Up @@ -72,14 +72,5 @@ def apply

it { should eql('#<Mutant::Matcher::Config ignore: [Bar] subjects: [Foo]>') }
end

context 'with subject filter' do
let(:object) do
described_class::DEFAULT
.add(:subject_filters, 'foo')
end

it { should eql('#<Mutant::Matcher::Config subject_filters: ["foo"]>') }
end
end
end
76 changes: 57 additions & 19 deletions spec/unit/mutant/matcher/method/instance_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -21,7 +22,7 @@
let(:env) do
instance_double(
Mutant::Env,
config: Mutant::Config::DEFAULT,
config: config,
parser: Fixtures::TEST_ENV.parser,
world: world
)
Expand Down Expand Up @@ -56,9 +57,62 @@ def arguments
end

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

it 'returns expected subjects' do
expect(subject).to eql([])
context 'on touching both diffs' do
let(:diff_a_touches?) { true }
let(:diff_b_touches?) { true }

include_examples 'a method matcher'
end
end
end

Expand All @@ -73,10 +127,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
Expand All @@ -90,10 +140,6 @@ def arguments
end

include_examples 'skipped candidate'

it 'returns expected subjects' do
expect(subject).to eql([])
end
end

context 'in module eval' do
Expand All @@ -106,10 +152,6 @@ def arguments
end

include_examples 'skipped candidate'

it 'returns expected subjects' do
expect(subject).to eql([])
end
end

context 'in class eval' do
Expand All @@ -122,10 +164,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
Expand Down
Loading