From 5aca4ccdfd11f00a91275a527561e80266bfe280 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 5 Oct 2018 16:29:46 -0400 Subject: [PATCH 1/9] Add a large result benchmark --- Rakefile | 6 ++++ benchmark/run.rb | 82 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/Rakefile b/Rakefile index d50ce5d97c..782d1b0777 100644 --- a/Rakefile +++ b/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 diff --git a/benchmark/run.rb b/benchmark/run.rb index 24968a6a85..5233c6287d 100644 --- a/benchmark/run.rb +++ b/benchmark/run.rb @@ -51,4 +51,86 @@ 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, {}) + 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 From 7c7947950f7dd0654842648b9729d0f3b56e1c0f Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 5 Oct 2018 16:57:31 -0400 Subject: [PATCH 2/9] Remove needless late-bound type check --- lib/graphql/execution/interpreter/runtime.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb index 2fd5aedc3b..4c3045b796 100644 --- a/lib/graphql/execution/interpreter/runtime.rb +++ b/lib/graphql/execution/interpreter/runtime.rb @@ -90,7 +90,6 @@ 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 From 5170bd30ed69322eef654abc6ab4c438cb1beecb Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 5 Oct 2018 16:58:19 -0400 Subject: [PATCH 3/9] Compare with strings to avoid slow #=== check --- lib/graphql/execution/interpreter/runtime.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb index 4c3045b796..57f9067a78 100644 --- a/lib/graphql/execution/interpreter/runtime.rb +++ b/lib/graphql/execution/interpreter/runtime.rb @@ -195,11 +195,11 @@ def continue_value(path, value, field, as_type, ast_node) 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) @@ -212,7 +212,7 @@ def continue_field(path, value, field, type, ast_node, next_selections) 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 @@ -225,10 +225,10 @@ def continue_field(path, value, field, type, ast_node, next_selections) 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| + value.each_with_index do |inner_value, idx| next_path = [*path, idx].freeze set_type_at_path(next_path, inner_type) after_lazy(inner_value, path: next_path, field: field) do |inner_inner_value| @@ -238,7 +238,7 @@ def continue_field(path, value, field, type, ast_node, next_selections) end end end - when TypeKinds::NON_NULL + when "NON_NULL" inner_type = type.of_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. From c5476b0e9065639d616db3496601ceb975c0bc8a Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 5 Oct 2018 16:58:50 -0400 Subject: [PATCH 4/9] Implement #get_field to avoid hash merging --- lib/graphql/execution/interpreter/runtime.rb | 2 +- lib/graphql/schema/member/has_fields.rb | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb index 57f9067a78..cfe2af7c15 100644 --- a/lib/graphql/execution/interpreter/runtime.rb +++ b/lib/graphql/execution/interpreter/runtime.rb @@ -94,7 +94,7 @@ def evaluate_selections(path, owner_object, owner_type, selections, root_operati 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)) diff --git a/lib/graphql/schema/member/has_fields.rb b/lib/graphql/schema/member/has_fields.rb index 2ebdd67e0f..c67ddb78b9 100644 --- a/lib/graphql/schema/member/has_fields.rb +++ b/lib/graphql/schema/member/has_fields.rb @@ -66,6 +66,15 @@ def fields all_fields end + def get_field(field_name) + for ancestor in ancestors + if ancestor.respond_to?(:own_fields) && f = ancestor.own_fields[field_name] + return f + end + end + nil + end + # Register this field with the class, overriding a previous one if needed. # Also, add a parent method for resolving this field. # @param field_defn [GraphQL::Schema::Field] @@ -123,7 +132,7 @@ def add_super_method(field_key, method_name) end default_resolve_module.module_eval <<-RUBY, __FILE__, __LINE__ + 1 def #{method_name}(**args) - field_inst = self.class.fields[#{field_key}] || raise(%|Failed to find field #{field_key} for \#{self.class} among \#{self.class.fields.keys}|) + field_inst = self.class.get_field(#{field_key}) || raise(%|Failed to find field #{field_key} for \#{self.class} among \#{self.class.fields.keys}|) field_inst.resolve_field_method(self, args, context) end RUBY From 81fd4e7822829228f242492c3de6e929f07ea638 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 5 Oct 2018 17:32:45 -0400 Subject: [PATCH 5/9] Tighten up writing logic --- .../execution/interpreter/hash_response.rb | 37 +++++----- lib/graphql/execution/interpreter/runtime.rb | 71 +++++++++++-------- 2 files changed, 56 insertions(+), 52 deletions(-) diff --git a/lib/graphql/execution/interpreter/hash_response.rb b/lib/graphql/execution/interpreter/hash_response.rb index 68214a468b..487338c308 100644 --- a/lib/graphql/execution/interpreter/hash_response.rb +++ b/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 diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb index cfe2af7c15..3821ad8d5c 100644 --- a/lib/graphql/execution/interpreter/runtime.rb +++ b/lib/graphql/execution/interpreter/runtime.rb @@ -154,26 +154,25 @@ def evaluate_selections(path, owner_object, owner_type, selections, root_operati end 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) } + elsif value.is_a?(GraphQL::ExecutionError) + value.path ||= path + value.ast_node ||= ast_node + write_execution_errors_in_response(path, [value]) + false + 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?) + write_execution_errors_in_response(path, value) false elsif value.is_a?(GraphQL::UnauthorizedError) # this hook might raise & crash, or it might return @@ -193,8 +192,6 @@ def continue_value(path, value, field, as_type, ast_node) end def continue_field(path, value, field, type, ast_node, next_selections) - type = resolve_if_late_bound_type(type) - case type.kind.name when "SCALAR", "ENUM" r = type.coerce_result(value, context) @@ -207,7 +204,7 @@ def continue_field(path, value, field, type, ast_node, next_selections) 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) @@ -228,7 +225,8 @@ def continue_field(path, value, field, type, ast_node, next_selections) when "LIST" write_in_response(path, []) inner_type = type.of_type - value.each_with_index do |inner_value, idx| + idx = 0 + value.each do |inner_value| next_path = [*path, idx].freeze set_type_at_path(next_path, inner_type) after_lazy(inner_value, path: next_path, field: field) do |inner_inner_value| @@ -237,9 +235,12 @@ def continue_field(path, value, field, type, ast_node, next_selections) continue_field(next_path, continue_value, field, inner_type, ast_node, next_selections) end end + idx += 1 end 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) @@ -385,27 +386,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 From c384aaa902023944ecd3af693d341fff79981e9e Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 5 Oct 2018 17:43:32 -0400 Subject: [PATCH 6/9] Add memory benchmark --- benchmark/run.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/benchmark/run.rb b/benchmark/run.rb index 5233c6287d..3afc758d03 100644 --- a/benchmark/run.rb +++ b/benchmark/run.rb @@ -63,12 +63,18 @@ def self.profile_large_result end result = RubyProf.profile do - schema.execute( document: document) + 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 From f0624690b11d7da1c82de0860164bad709af2dcd Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 5 Oct 2018 17:55:28 -0400 Subject: [PATCH 7/9] Simplify creation of next_path --- lib/graphql/execution/interpreter/runtime.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb index 3821ad8d5c..e6c326dd76 100644 --- a/lib/graphql/execution/interpreter/runtime.rb +++ b/lib/graphql/execution/interpreter/runtime.rb @@ -110,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` @@ -227,7 +230,9 @@ def continue_field(path, value, field, type, ast_node, next_selections) inner_type = type.of_type idx = 0 value.each do |inner_value| - next_path = [*path, idx].freeze + 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) From 41aac405fd96b27afe51b77f43109107bd2f04d4 Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 5 Oct 2018 17:55:50 -0400 Subject: [PATCH 8/9] Don't use multiple return, since it allocates an Array --- lib/graphql/execution/interpreter/runtime.rb | 23 ++++++++++---------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb index e6c326dd76..d7b9db9280 100644 --- a/lib/graphql/execution/interpreter/runtime.rb +++ b/lib/graphql/execution/interpreter/runtime.rb @@ -148,14 +148,15 @@ 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? if as_type.non_null? @@ -164,19 +165,19 @@ def continue_value(path, value, field, as_type, ast_node) else write_in_response(path, nil) end - false + HALT elsif value.is_a?(GraphQL::ExecutionError) value.path ||= path value.ast_node ||= ast_node write_execution_errors_in_response(path, [value]) - false + 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_execution_errors_in_response(path, value) - false + HALT elsif value.is_a?(GraphQL::UnauthorizedError) # this hook might raise & crash, or it might return # a replacement value @@ -188,9 +189,9 @@ 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 @@ -219,8 +220,8 @@ def continue_field(path, value, field, type, ast_node, next_selections) 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 @@ -235,8 +236,8 @@ def continue_field(path, value, field, type, ast_node, next_selections) 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 From a7553e082aa45c6c1675cc5c025ad1c429d4f10a Mon Sep 17 00:00:00 2001 From: Robert Mosolgo Date: Fri, 5 Oct 2018 17:56:06 -0400 Subject: [PATCH 9/9] make fewer arguments hashes --- lib/graphql/execution/interpreter/runtime.rb | 5 +++-- lib/graphql/schema/member/has_arguments.rb | 8 ++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/graphql/execution/interpreter/runtime.rb b/lib/graphql/execution/interpreter/runtime.rb index d7b9db9280..e8dc38523f 100644 --- a/lib/graphql/execution/interpreter/runtime.rb +++ b/lib/graphql/execution/interpreter/runtime.rb @@ -311,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) @@ -325,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 diff --git a/lib/graphql/schema/member/has_arguments.rb b/lib/graphql/schema/member/has_arguments.rb index a3722ac9d3..66728d30a8 100644 --- a/lib/graphql/schema/member/has_arguments.rb +++ b/lib/graphql/schema/member/has_arguments.rb @@ -29,9 +29,13 @@ def add_argument(arg_defn) # @return [Hash 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