Low hanging fruit in interpreter

Rakefile

@@ -92,6 +92,12 @@ namespace :bench do
     prepare_benchmark
     GraphQLBenchmark.profile
   end
+
+  desc "Run benchmarks on a very large result"
+  task :profile_large_result do
+    prepare_benchmark
+    GraphQLBenchmark.profile_large_result
+  end
 end
 
 namespace :test do

benchmark/run.rb

@@ -51,4 +51,92 @@ def self.profile
 
     printer.print(STDOUT, {})
   end
+
+  # Adapted from https://github.com/rmosolgo/graphql-ruby/issues/861
+  def self.profile_large_result
+    schema = ProfileLargeResult::Schema
+    document = ProfileLargeResult::ALL_FIELDS
+    Benchmark.ips do |x|
+      x.report("Querying for #{ProfileLargeResult::DATA.size} objects") {
+        schema.execute(document: document)
+      }
+    end
+
+    result = RubyProf.profile do
+      schema.execute(document: document)
+    end
+    printer = RubyProf::FlatPrinter.new(result)
+    # printer = RubyProf::GraphHtmlPrinter.new(result)
+    # printer = RubyProf::FlatPrinterWithLineNumbers.new(result)
+    printer.print(STDOUT, {})
+
+    report = MemoryProfiler.report do
+      schema.execute(document: document)
+    end
+
+    report.pretty_print
+  end
+
+  module ProfileLargeResult
+    DATA = 1000.times.map {
+      {
+        id:             SecureRandom.uuid,
+        int1:           SecureRandom.random_number(100000),
+        int2:           SecureRandom.random_number(100000),
+        string1:        SecureRandom.base64,
+        string2:        SecureRandom.base64,
+        boolean1:       SecureRandom.random_number(1) == 0,
+        boolean2:       SecureRandom.random_number(1) == 0,
+        int_array:      10.times.map { SecureRandom.random_number(100000) },
+        string_array:   10.times.map { SecureRandom.base64 },
+        boolean_array:  10.times.map { SecureRandom.random_number(1) == 0 },
+      }
+    }
+
+
+    class FooType < GraphQL::Schema::Object
+      field :id, ID, null: false
+      field :int1, Integer, null: false
+      field :int2, Integer, null: false
+      field :string1, String, null: false
+      field :string2, String, null: false
+      field :boolean1, Boolean, null: false
+      field :boolean2, Boolean, null: false
+      field :string_array, [String], null: false
+      field :int_array, [Integer], null: false
+      field :boolean_array, [Boolean], null: false
+    end
+
+    class QueryType < GraphQL::Schema::Object
+      description "Query root of the system"
+      field :foos, [FooType], null: false, description: "Return a list of Foo objects"
+      def foos
+        DATA
+      end
+    end
+
+    class Schema < GraphQL::Schema
+      query QueryType
+      if TESTING_INTERPRETER
+        use GraphQL::Execution::Interpreter
+      end
+    end
+
+    ALL_FIELDS = GraphQL.parse <<-GRAPHQL
+      {
+        foos {
+          id
+          int1
+          int2
+          string1
+          string2
+          boolean1
+          boolean2
+          stringArray
+          intArray
+          booleanArray
+        }
+      }
+    GRAPHQL
+  end
 end

lib/graphql/execution/interpreter/hash_response.rb

@@ -20,29 +20,24 @@ def inspect
 
         # Add `value` at `path`.
         # @return [void]
-        def write(path, value, propagating_nil: false)
-          write_target = @result
-          if write_target
-            if path.none?
-              @result = value
-            else
-              path.each_with_index do |path_part, idx|
-                next_part = path[idx + 1]
-                if next_part.nil?
-                  if write_target[path_part].nil? || (propagating_nil)
-                    write_target[path_part] = value
-                  else
-                    raise "Invariant: Duplicate write to #{path} (previous: #{write_target[path_part].inspect}, new: #{value.inspect})"
-                  end
-                else
-                  # Don't have to worry about dead paths here
-                  # because it's tracked by the runtime,
-                  # and values for dead paths are not sent to this method.
-                  write_target = write_target.fetch(path_part, :__unset)
-                end
-              end
+        def write(path, value)
+          if path.size == 0 # faster than #none?
+            @result = value
+          elsif (write_target = @result)
+            i = 0
+            prefinal_steps = path.size - 1
+            # Use `while` to avoid a closure
+            while i < prefinal_steps
+              path_part = path[i]
+              i += 1
+              write_target = write_target[path_part]
             end
+            path_part = path[i]
+            write_target[path_part] = value
+          else
+            # The response is completely nulled out
           end
+
           nil
         end
       end

lib/graphql/execution/interpreter/runtime.rb

@@ -90,12 +90,11 @@ def gather_selections(owner_type, selections, selections_by_name)
 
         def evaluate_selections(path, owner_object, owner_type, selections, root_operation_type: nil)
           selections_by_name = {}
-          owner_type = resolve_if_late_bound_type(owner_type)
           gather_selections(owner_type, selections, selections_by_name)
           selections_by_name.each do |result_name, fields|
             ast_node = fields.first
             field_name = ast_node.name
-            field_defn = owner_type.fields[field_name]
+            field_defn = owner_type.get_field(field_name)
             is_introspection = false
             if field_defn.nil?
               field_defn = if owner_type == schema.query.metadata[:type_class] && (entry_point_field = schema.introspection_system.entry_point(name: field_name))
@@ -111,7 +110,10 @@ def evaluate_selections(path, owner_object, owner_type, selections, root_operati
 
             return_type = resolve_if_late_bound_type(field_defn.type)
 
-            next_path = [*path, result_name].freeze
+            next_path = path.dup
+            next_path << result_name
+            next_path.freeze
+
             # This seems janky, but we need to know
             # the field's return type at this path in order
             # to propagate `null`
@@ -146,36 +148,36 @@ def evaluate_selections(path, owner_object, owner_type, selections, root_operati
             end
 
             after_lazy(app_result, field: field_defn, path: next_path, eager: root_operation_type == "mutation") do |inner_result|
-              should_continue, continue_value = continue_value(next_path, inner_result, field_defn, return_type, ast_node)
-              if should_continue
+              continue_value = continue_value(next_path, inner_result, field_defn, return_type, ast_node)
+              if HALT != continue_value
                 continue_field(next_path, continue_value, field_defn, return_type, ast_node, next_selections)
               end
             end
           end
         end
 
+        HALT = Object.new
         def continue_value(path, value, field, as_type, ast_node)
-          if value.nil? || value.is_a?(GraphQL::ExecutionError)
-            if value.nil?
-              if as_type.non_null?
-                err = GraphQL::InvalidNullError.new(field.owner, field, value)
-                write_in_response(path, err, propagating_nil: true)
-              else
-                write_in_response(path, nil)
-              end
+          if value.nil?
+            if as_type.non_null?
+              err = GraphQL::InvalidNullError.new(field.owner, field, value)
+              write_invalid_null_in_response(path, err)
             else
-              value.path ||= path
-              value.ast_node ||= ast_node
-              write_in_response(path, value, propagating_nil: as_type.non_null?)
+              write_in_response(path, nil)
             end
-            false
-          elsif value.is_a?(Array) && value.all? { |v| v.is_a?(GraphQL::ExecutionError) }
+            HALT
+          elsif value.is_a?(GraphQL::ExecutionError)
+            value.path ||= path
+            value.ast_node ||= ast_node
+            write_execution_errors_in_response(path, [value])
+            HALT
+          elsif value.is_a?(Array) && value.any? && value.all? { |v| v.is_a?(GraphQL::ExecutionError) }
             value.each do |v|
               v.path ||= path
               v.ast_node ||= ast_node
             end
-            write_in_response(path, value, propagating_nil: as_type.non_null?)
-            false
+            write_execution_errors_in_response(path, value)
+            HALT
           elsif value.is_a?(GraphQL::UnauthorizedError)
             # this hook might raise & crash, or it might return
             # a replacement value
@@ -187,60 +189,64 @@ def continue_value(path, value, field, as_type, ast_node)
 
             continue_value(path, next_value, field, as_type, ast_node)
           elsif GraphQL::Execution::Execute::SKIP == value
-            false
+            HALT
           else
-            return true, value
+            value
           end
         end
 
         def continue_field(path, value, field, type, ast_node, next_selections)
-          type = resolve_if_late_bound_type(type)
-
-          case type.kind
-          when TypeKinds::SCALAR, TypeKinds::ENUM
+          case type.kind.name
+          when "SCALAR", "ENUM"
             r = type.coerce_result(value, context)
             write_in_response(path, r)
-          when TypeKinds::UNION, TypeKinds::INTERFACE
+          when "UNION", "INTERFACE"
             resolved_type = query.resolve_type(type, value)
             possible_types = query.possible_types(type)
 
             if !possible_types.include?(resolved_type)
               parent_type = field.owner
               type_error = GraphQL::UnresolvedTypeError.new(value, field, parent_type, resolved_type, possible_types)
               schema.type_error(type_error, context)
-              write_in_response(path, nil, propagating_nil: field.type.non_null?)
+              write_in_response(path, nil)
             else
               resolved_type = resolved_type.metadata[:type_class]
               continue_field(path, value, field, resolved_type, ast_node, next_selections)
             end
-          when TypeKinds::OBJECT
+          when "OBJECT"
             object_proxy = begin
               type.authorized_new(value, context)
             rescue GraphQL::ExecutionError => err
               err
             end
             after_lazy(object_proxy, path: path, field: field) do |inner_object|
-              should_continue, continue_value = continue_value(path, inner_object, field, type, ast_node)
-              if should_continue
+              continue_value = continue_value(path, inner_object, field, type, ast_node)
+              if HALT != continue_value
                 write_in_response(path, {})
                 evaluate_selections(path, continue_value, type, next_selections)
               end
             end
-          when TypeKinds::LIST
+          when "LIST"
             write_in_response(path, [])
             inner_type = type.of_type
-            value.each_with_index.each do |inner_value, idx|
-              next_path = [*path, idx].freeze
+            idx = 0
+            value.each do |inner_value|
+              next_path = path.dup
+              next_path << idx
+              next_path.freeze
               set_type_at_path(next_path, inner_type)
               after_lazy(inner_value, path: next_path, field: field) do |inner_inner_value|
-                should_continue, continue_value = continue_value(next_path, inner_inner_value, field, inner_type, ast_node)
-                if should_continue
+                continue_value = continue_value(next_path, inner_inner_value, field, inner_type, ast_node)
+                if HALT != continue_value
                   continue_field(next_path, continue_value, field, inner_type, ast_node, next_selections)
                 end
               end
+              idx += 1
             end
-          when TypeKinds::NON_NULL
+          when "NON_NULL"
             inner_type = type.of_type
+            # For fields like `__schema: __Schema!`
+            inner_type = resolve_if_late_bound_type(inner_type)
             # Don't `set_type_at_path` because we want the static type,
             # we're going to use that to determine whether a `nil` should be propagated or not.
             continue_field(path, value, field, inner_type, ast_node, next_selections)
@@ -305,8 +311,9 @@ def after_lazy(obj, field:, path:, eager: false)
 
         def arguments(graphql_object, arg_owner, ast_node)
           kwarg_arguments = {}
+          arg_defns = arg_owner.arguments
           ast_node.arguments.each do |arg|
-            arg_defn = arg_owner.arguments[arg.name]
+            arg_defn = arg_defns[arg.name]
             # Need to distinguish between client-provided `nil`
             # and nothing-at-all
             is_present, value = arg_to_value(graphql_object, arg_defn.type, arg.value)
@@ -319,7 +326,7 @@ def arguments(graphql_object, arg_owner, ast_node)
               kwarg_arguments[arg_defn.keyword] = value
             end
           end
-          arg_owner.arguments.each do |name, arg_defn|
+          arg_defns.each do |name, arg_defn|
             if arg_defn.default_value? && !kwarg_arguments.key?(arg_defn.keyword)
               kwarg_arguments[arg_defn.keyword] = arg_defn.default_value
             end
@@ -386,27 +393,35 @@ def flatten_ast_value(v)
           end
         end
 
-        def write_in_response(path, value, propagating_nil: false)
+        def write_invalid_null_in_response(path, invalid_null_error)
+          if !dead_path?(path)
+            schema.type_error(invalid_null_error, context)
+            write_in_response(path, nil)
+            add_dead_path(path)
+          end
+        end
+
+        def write_execution_errors_in_response(path, errors)
+          if !dead_path?(path)
+            errors.each do |v|
+              context.errors << v
+            end
+            write_in_response(path, nil)
+            add_dead_path(path)
+          end
+        end
+
+        def write_in_response(path, value)
           if dead_path?(path)
             return
           else
-            if value.is_a?(GraphQL::ExecutionError) || (value.is_a?(Array) && value.any? && value.all? { |v| v.is_a?(GraphQL::ExecutionError)})
-              Array(value).each do |v|
-                context.errors << v
-              end
-              write_in_response(path, nil, propagating_nil: propagating_nil)
-              add_dead_path(path)
-            elsif value.is_a?(GraphQL::InvalidNullError)
-              schema.type_error(value, context)
-              write_in_response(path, nil, propagating_nil: true)
-              add_dead_path(path)
-            elsif value.nil? && path.any? && type_at(path).non_null?
+            if value.nil? && path.any? && type_at(path).non_null?
               # This nil is invalid, try writing it at the previous spot
               propagate_path = path[0..-2]
-              write_in_response(propagate_path, value, propagating_nil: true)
+              write_in_response(propagate_path, value)
               add_dead_path(propagate_path)
             else
-              @response.write(path, value, propagating_nil: propagating_nil)
+              @response.write(path, value)
             end
           end
         end

lib/graphql/schema/member/has_arguments.rb

@@ -29,9 +29,13 @@ def add_argument(arg_defn)
 
         # @return [Hash<String => GraphQL::Schema::Argument] Arguments defined on this thing, keyed by name. Includes inherited definitions
         def arguments
-          inherited_arguments = ((self.is_a?(Class) && superclass.respond_to?(:arguments)) ? superclass.arguments : {})
+          inherited_arguments = ((self.is_a?(Class) && superclass.respond_to?(:arguments)) ? superclass.arguments : nil)
           # Local definitions override inherited ones
-          inherited_arguments.merge(own_arguments)
+          if inherited_arguments
+            inherited_arguments.merge(own_arguments)
+          else
+            own_arguments
+          end
         end
 
         # @param new_arg_class [Class] A class to use for building argument definitions