From 2c97593be8b84a111b66662bb4f695a2342d228a Mon Sep 17 00:00:00 2001 From: schneems Date: Mon, 11 Oct 2021 13:39:57 -0500 Subject: [PATCH] Reduce timeout to 1 second There was a concern about the appearance of a 5 second timeout. In my experience this is quite a high limit. https://bugs.ruby-lang.org/issues/18159#note-7. When I set the original value I didn't know what performance characteristics we would have and 5 seconds seemed like a large amount of time to recover from slowness. In practice the times we've had performance issues are due to cases where there are bugs that will either never exit #77 or due to a flaw in some subset of the logic that would otherwise take exponential time to resolve #35. Basically: If we cannot resolve a document in 1 second, there's very little chance we'll be able to figure it out in the next 4. I feel reasonably confident about dropping it down to 1. The hot path continues to be Ripper and re-parsing the document constantly. However overall it seems very performant. Also I'm keeping an eye on the `combination` logic as it might come back to be a problem for pathological cases where there are many syntax-errors rather than 1 or 2. At this time, the strategy is to wait for performance issues to occur and then tackle them. It doesn't seem to be a huge problem at the moment. Moving the performance test to a regression test as all tests must now execute under 1 second or the timeout will cause them to fail. We can consider introducing some FYI benchmarks, perhaps seeing how many IPS (Iterations Per Second) we can do on one case over 5 seconds. But not gate or fail on it. Just keep it around as an optimization target. But if that's the case, maybe it's fine to wait until do that optimization work. --- CHANGELOG.md | 1 + lib/dead_end/internals.rb | 2 +- .../improvement_regression_spec.rb | 19 ++++++++++++ spec/perf/perf_spec.rb | 31 ------------------- 4 files changed, 21 insertions(+), 32 deletions(-) delete mode 100644 spec/perf/perf_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index b716f8f..be01d2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## HEAD (unreleased) +- Reduce timeout to 1 second (https://github.com/zombocom/dead_end/pull/79) - Logically consecutive lines (such as chained methods are now joined) (https://github.com/zombocom/dead_end/pull/78) - Output improvement for cases where the only line is an single `end` (https://github.com/zombocom/dead_end/pull/78) diff --git a/lib/dead_end/internals.rb b/lib/dead_end/internals.rb index 6a79845..8dd1baf 100644 --- a/lib/dead_end/internals.rb +++ b/lib/dead_end/internals.rb @@ -14,7 +14,7 @@ module DeadEnd class Error < StandardError; end SEARCH_SOURCE_ON_ERROR_DEFAULT = true - TIMEOUT_DEFAULT = ENV.fetch("DEAD_END_TIMEOUT", 5).to_i + TIMEOUT_DEFAULT = ENV.fetch("DEAD_END_TIMEOUT", 1).to_i def self.handle_error(e, search_source_on_error: SEARCH_SOURCE_ON_ERROR_DEFAULT) raise e unless e.message.include?("end-of-input") diff --git a/spec/integration/improvement_regression_spec.rb b/spec/integration/improvement_regression_spec.rb index 6934756..07423b5 100644 --- a/spec/integration/improvement_regression_spec.rb +++ b/spec/integration/improvement_regression_spec.rb @@ -4,6 +4,25 @@ module DeadEnd RSpec.describe "Library only integration to test regressions and improvements" do + it "returns good results on routes.rb" do + source = fixtures_dir.join("routes.rb.txt").read + + io = StringIO.new + DeadEnd.call( + io: io, + source: source, + filename: "none" + ) + + expect(io.string).to include(<<~'EOM'.indent(4)) + 1 Rails.application.routes.draw do + ❯ 113 namespace :admin do + ❯ 116 match "/foobar(*path)", via: :all, to: redirect { |_params, req| + ❯ 120 } + 121 end + EOM + end + it "handles multi-line-methods issues/64" do source = fixtures_dir.join("webmock.rb.txt").read diff --git a/spec/perf/perf_spec.rb b/spec/perf/perf_spec.rb deleted file mode 100644 index 221d5ba..0000000 --- a/spec/perf/perf_spec.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -require_relative "../spec_helper" -require "benchmark" - -module DeadEnd - RSpec.describe "perf" do - it "doesnt timeout" do - source = fixtures_dir.join("routes.rb.txt").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 - ❯ 113 namespace :admin do - ❯ 116 match "/foobar(*path)", via: :all, to: redirect { |_params, req| - ❯ 120 } - 121 end - EOM - - expect(bench.real).to be < 1 # second - end - end -end