Merge pull request #604 from bugsnag/code-extraction-performance-impr…

…ovements

Code extraction performance improvements

lib/bugsnag/code_extractor.rb

@@ -0,0 +1,137 @@
+module Bugsnag
+  # @api private
+  class CodeExtractor
+    MAXIMUM_LINES_TO_KEEP = 7
+
+    ##
+    # @param configuration [Configuration]
+    def initialize(configuration)
+      @files = {}
+      @configuration = configuration
+    end
+
+    ##
+    # Add a file and its corresponding trace hash to be processed.
+    #
+    # @param path [String] The full path to the file
+    # @param trace [Hash]
+    # @return [void]
+    def add_file(path, trace)
+      # If the file doesn't exist we can't extract code from it, so we can skip
+      # this file entirely
+      unless File.exist?(path)
+        trace[:code] = nil
+
+        return
+      end
+
+      @files[path] ||= []
+      @files[path].push(trace)
+
+      # Record the line numbers we want to fetch for this trace
+      # We grab extra lines so that we can compensate if the error is on the
+      # first or last line of a file
+      first_line_number = trace[:lineNumber] - MAXIMUM_LINES_TO_KEEP
+
+      trace[:first_line_number] = first_line_number < 1 ? 1 : first_line_number
+      trace[:last_line_number] = trace[:lineNumber] + MAXIMUM_LINES_TO_KEEP
+    end
+
+    ##
+    # Add the code to the hashes that were given in {#add_file} by modifying
+    # them in-place. They will have a new ':code' key containing a hash of line
+    # number => string of code for that line
+    #
+    # @return [void]
+    def extract!
+      @files.each do |path, traces|
+        begin
+          line_numbers = Set.new
+
+          traces.each do |trace|
+            trace[:first_line_number].upto(trace[:last_line_number]) do |line_number|
+              line_numbers << line_number
+            end
+          end
+
+          extract_from(path, traces, line_numbers)
+        rescue StandardError => e
+          # Clean up after ourselves
+          traces.each do |trace|
+            trace[:code] ||= nil
+            trace.delete(:first_line_number)
+            trace.delete(:last_line_number)
+          end
+
+          @configuration.warn("Error extracting code: #{e.inspect}")
+        end
+      end
+    end
+
+    private
+
+    ##
+    # @param path [String]
+    # @param traces [Array<Hash>]
+    # @param line_numbers [Set<Integer>]
+    # @return [void]
+    def extract_from(path, traces, line_numbers)
+      code = {}
+
+      File.open(path) do |file|
+        current_line_number = 0
+
+        file.each_line do |line|
+          current_line_number += 1
+
+          next unless line_numbers.include?(current_line_number)
+
+          code[current_line_number] = line[0...200].rstrip
+        end
+      end
+
+      associate_code_with_trace(code, traces)
+    end
+
+    ##
+    # @param code [Hash{Integer => String}]
+    # @param traces [Array<Hash>]
+    # @return [void]
+    def associate_code_with_trace(code, traces)
+      traces.each do |trace|
+        trace[:code] = {}
+
+        code.each do |line_number, line|
+          # If we've gone past the last line we care about, we can stop iterating
+          break if line_number > trace[:last_line_number]
+
+          # Skip lines that aren't in the range we want
+          next unless line_number >= trace[:first_line_number]
+
+          trace[:code][line_number] = line
+        end
+
+        trim_excess_lines(trace[:code], trace[:lineNumber])
+        trace.delete(:first_line_number)
+        trace.delete(:last_line_number)
+      end
+    end
+
+    ##
+    # @param code [Hash{Integer => String}]
+    # @param line_number [Integer]
+    # @return [void]
+    def trim_excess_lines(code, line_number)
+      while code.length > MAXIMUM_LINES_TO_KEEP
+        last_line = code.keys.max
+        first_line = code.keys.min
+
+        if (last_line - line_number) > (line_number - first_line)
+          code.delete(last_line)
+        else
+          code.delete(first_line)
+        end
+      end
+    end
+  end
+end

lib/bugsnag/report.rb

@@ -182,7 +182,7 @@ def generate_exception_list
         {
           errorClass: error_class(exception),
           message: exception.message,
-          stacktrace: Stacktrace.new(exception.backtrace, configuration).to_a
+          stacktrace: Stacktrace.process(exception.backtrace, configuration)
         }
       end
     end

lib/bugsnag/stacktrace.rb

@@ -1,6 +1,7 @@
-module Bugsnag
-  class Stacktrace
+require_relative 'code_extractor'
 
+module Bugsnag
+  module Stacktrace
     # e.g. "org/jruby/RubyKernel.java:1264:in `catch'"
     BACKTRACE_LINE_REGEX = /^((?:[a-zA-Z]:)?[^:]+):(\d+)(?::in `([^']+)')?$/
 
@@ -10,41 +11,41 @@ class Stacktrace
     ##
     # Process a backtrace and the configuration into a parsed stacktrace.
     #
-    # rubocop:todo Metrics/CyclomaticComplexity
-    def initialize(backtrace, configuration)
-      @configuration = configuration
+    # @param backtrace [Array, nil] If nil, 'caller' will be used instead
+    # @param configuration [Configuration]
+    # @return [Array]
+    def self.process(backtrace, configuration)
+      code_extractor = CodeExtractor.new(configuration)
 
       backtrace = caller if !backtrace || backtrace.empty?
 
-      @processed_backtrace = backtrace.map do |trace|
+      processed_backtrace = backtrace.map do |trace|
         # Parse the stacktrace line
         if trace.match(BACKTRACE_LINE_REGEX)
           file, line_str, method = [$1, $2, $3]
         elsif trace.match(JAVA_BACKTRACE_REGEX)
           method, file, line_str = [$1, $2, $3]
         end
 
-        next(nil) if file.nil?
+        next if file.nil?
 
         # Expand relative paths
         file = File.realpath(file) rescue file
 
         # Generate the stacktrace line hash
-        trace_hash = {}
-        trace_hash[:lineNumber] = line_str.to_i
+        trace_hash = { lineNumber: line_str.to_i }
 
-        if configuration.send_code
-          trace_hash[:code] = code(file, trace_hash[:lineNumber])
-        end
+        # Save a copy of the file path as we're about to modify it but need the
+        # raw version when extracting code (otherwise we can't open the file)
+        raw_file_path = file.dup
 
         # Clean up the file path in the stacktrace
-        if defined?(@configuration.project_root) && @configuration.project_root.to_s != ''
-          trace_hash[:inProject] = true if file.start_with?(@configuration.project_root.to_s)
-          file.sub!(/#{@configuration.project_root}\//, "")
-          trace_hash.delete(:inProject) if file.match(@configuration.vendor_path)
+        if defined?(configuration.project_root) && configuration.project_root.to_s != ''
+          trace_hash[:inProject] = true if file.start_with?(configuration.project_root.to_s)
+          file.sub!(/#{configuration.project_root}\//, "")
+          trace_hash.delete(:inProject) if file.match(configuration.vendor_path)
         end
 
-
         # Strip common gem path prefixes
         if defined?(Gem)
           file = Gem.path.inject(file) {|line, path| line.sub(/#{path}\//, "") }
@@ -55,60 +56,16 @@ def initialize(backtrace, configuration)
         # Add a method if we have it
         trace_hash[:method] = method if method && (method =~ /^__bind/).nil?
 
-        if trace_hash[:file] && !trace_hash[:file].empty?
-          trace_hash
-        else
-          nil
-        end
-      end.compact
-    end
-    # rubocop:enable Metrics/CyclomaticComplexity
-
-    ##
-    # Returns the processed backtrace
-    def to_a
-      @processed_backtrace
-    end
-
-    private
+        # If we're going to send code then record the raw file path and the
+        # trace_hash, so we can extract from it later
+        code_extractor.add_file(raw_file_path, trace_hash) if configuration.send_code
 
-    def code(file, line_number, num_lines = 7)
-      code_hash = {}
-
-      from_line = [line_number - num_lines, 1].max
-
-      # don't try and open '(irb)' or '-e'
-      return unless File.exist?(file)
-
-      # Populate code hash with line numbers and code lines
-      File.open(file) do |f|
-        current_line_number = 0
-        f.each_line do |line|
-          current_line_number += 1
-
-          next if current_line_number < from_line
-
-          code_hash[current_line_number] = line[0...200].rstrip
-
-          break if code_hash.length >= ( num_lines * 1.5 ).ceil
-        end
-      end
-
-      while code_hash.length > num_lines
-        last_line = code_hash.keys.max
-        first_line = code_hash.keys.min
+        trace_hash
+      end.compact
 
-        if (last_line - line_number) > (line_number - first_line)
-          code_hash.delete(last_line)
-        else
-          code_hash.delete(first_line)
-        end
-      end
+      code_extractor.extract! if configuration.send_code
 
-      code_hash
-    rescue
-      @configuration.warn("Error fetching code: #{$!.inspect}")
-      nil
+      processed_backtrace
     end
   end
 end