From 5656caa3116f38b1d1b3ef139dc4c692d93d04b9 Mon Sep 17 00:00:00 2001 From: schneems Date: Fri, 11 Dec 2020 13:42:09 -0600 Subject: [PATCH] Fix perf issue with combination After dead_end has determined that the problem has been found, it then goes back through all it's code blocks to determine the smallest set of blocks which hold all the errors (since there can be multiple errors and multiple blocks could hold the same error). It does this by building a "combination" of all possible block results and then checking each. To see, look at this spec: ```ruby expect( CodeFrontier.combination([:a, :b, :c, :d]) ).to eq( [ [:a],[:b],[:c],[:d], [:a, :b], [:a, :c], [:a, :d], [:b, :c], [:b, :d], [:c, :d], [:a, :b, :c], [:a, :b, :d], [:a, :c, :d], [:b, :c, :d], [:a, :b, :c, :d] ] ) ``` Imagine that :a, :b, :c, and :d are each code blocks. If we have 4 blocks in our frontier it generates 15 possibilities to evaluate. - 4 => 15 - 5 => 31 - 6 => 63 - 7 => 127 In this failing scenario there are a large number of blocks 23 blocks to be exact. - 23 => 8,388,607 It takes 4.7 seconds just to generate all these combinations with numeric values, which is not sustainable. This doesn't even include the time to perform the parse check 8 million times, just to generate 8 million arrays with all possible combinations. YIKEs. This wasn't previously as issue because as code blocks "expand" they consolidate when a larger block contains smaller blocks they get grouped together. The reason this example is so "bad", is that there's a lot of blocks all at the same indentation level, so they've not had a chance to consolidate yet. The fix is to evaluate fewer combinations. We know that removing valid code from an invalid document cannot create a valid document, so we can remove all blocks that are already valid. When we've filtered to only invalid blocks, there's only 1 in the frontier which comes back almost instantaneously. This patch reduces the time to find the syntax error from about 9 seconds to 0.03 seconds. Not bad --- CHANGELOG.md | 2 + lib/dead_end/code_frontier.rb | 2 +- lib/dead_end/internals.rb | 3 +- spec/fixtures/routes.txt.rb | 121 ++++++++++++++++++++++++++++++++++ spec/perf/perf_spec.rb | 33 ++++++++++ 5 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 spec/fixtures/routes.txt.rb create mode 100644 spec/perf/perf_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d7ddb8..24f2b19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## HEAD (unreleased) +- Fix performance issue when evaluating multiple block combinations (https://github.com/zombocom/dead_end/pull/35) + ## 1.0.0 - Gem name changed from `syntax_search` to `dead_end` (https://github.com/zombocom/syntax_search/pull/30) diff --git a/lib/dead_end/code_frontier.rb b/lib/dead_end/code_frontier.rb index e5659d6..1ba6291 100644 --- a/lib/dead_end/code_frontier.rb +++ b/lib/dead_end/code_frontier.rb @@ -143,7 +143,7 @@ def self.combination(array) # Given that we know our syntax error exists somewhere in our frontier, we want to find # the smallest possible set of blocks that contain all the syntax errors def detect_invalid_blocks - self.class.combination(@frontier).detect do |block_array| + self.class.combination(@frontier.select(&:invalid?)).detect do |block_array| holds_all_syntax_errors?(block_array) end || [] end diff --git a/lib/dead_end/internals.rb b/lib/dead_end/internals.rb index 92afbcc..ddd5023 100644 --- a/lib/dead_end/internals.rb +++ b/lib/dead_end/internals.rb @@ -53,8 +53,9 @@ def self.call(source: , filename: , terminal: false, record_dir: nil, timeout: T invalid_obj: invalid_type(source), io: io ).call - rescue Timeout::Error + rescue Timeout::Error => e io.puts "Search timed out DEAD_END_TIMEOUT=#{timeout}, run with DEBUG=1 for more info" + io.puts e.backtrace.first(3).join($/) end # Used for counting spaces diff --git a/spec/fixtures/routes.txt.rb b/spec/fixtures/routes.txt.rb new file mode 100644 index 0000000..c4c7a16 --- /dev/null +++ b/spec/fixtures/routes.txt.rb @@ -0,0 +1,121 @@ +Rails.application.routes.draw do + constraints -> { Rails.application.config.non_production } do + namespace :foo do + resource :bar + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + constraints -> { Rails.application.config.non_production } do + namespace :bar do + resource :baz + end + end + + namespace :admin do + resource :session + + match "/out_of_office(*path)", via: :all, to: redirect { |_params, req| + uri = URI(req.path.gsub("out_of_office", "in_office")) + uri.query = req.query_string.presence + uri.to_s + } +end diff --git a/spec/perf/perf_spec.rb b/spec/perf/perf_spec.rb new file mode 100644 index 0000000..ffdc956 --- /dev/null +++ b/spec/perf/perf_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require_relative "../spec_helper.rb" +require "benchmark" + +module DeadEnd + RSpec.describe "perf" do + it "doesnt timeout" do + source = fixtures_dir.join("routes.txt.rb").read + + io = StringIO.new + bench = Benchmark.measure do + DeadEnd.call( + io: io, + source: source, + filename: "none", + ) + end + + expect(io.string).to include(<<~'EOM'.indent(4)) + 1 Rails.application.routes.draw do + 107 constraints -> { Rails.application.config.non_production } do + 111 end + ❯ 113 namespace :admin do + ❯ 116 match "/out_of_office(*path)", via: :all, to: redirect { |_params, req| + ❯ 120 } + 121 end + EOM + + expect(bench.real).to be < 1 # second + end + end +end