Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(graphql): cache attribute hashes #723

Merged
merged 7 commits into from
Nov 27, 2023
Merged

perf(graphql): cache attribute hashes #723

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fff3db5
GraphQL: cache attribute hashes
rmosolgo Nov 21, 2023
c19736b
Merge branch 'main' into reduce-graphql-allocations
arielvalentin Nov 22, 2023
4c140f8
Merge branch 'main' into reduce-graphql-allocations
arielvalentin Nov 22, 2023
92ad82f
Use caches for GraphQLTracer attributes
rmosolgo Nov 22, 2023
e65ef47
Use the same attribute hashes for resolve_type and authorized
rmosolgo Nov 22, 2023
2c58338
revert changes to legacy trace
rmosolgo Nov 27, 2023
0ff339a
Merge branch 'main' into reduce-graphql-allocations
arielvalentin Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 61 additions & 33 deletions instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_trace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,61 @@ module GraphQLTrace # rubocop:disable Metrics/ModuleLength
def initialize(trace_scalars: false, **_options)
@trace_scalars = trace_scalars
@_otel_field_key_cache = Hash.new { |h, k| h[k] = _otel_field_key(k) }
@_otel_field_key_cache.compare_by_identity
@_otel_authorized_key_cache = Hash.new { |h, k| h[k] = _otel_authorized_key(k) }
@_otel_authorized_key_cache.compare_by_identity
@_otel_resolve_type_key_cache = Hash.new { |h, k| h[k] = _otel_resolve_type_key(k) }
@_otel_resolve_type_key_cache.compare_by_identity

@_otel_authorized_attrs_cache = Hash.new do |h, type|
h[type] = {
'graphql.type.name' => type.graphql_name,
'graphql.lazy' => false
}.freeze
end
@_otel_authorized_attrs_cache.compare_by_identity

@_otel_lazy_authorized_attrs_cache = Hash.new do |h, type|
h[type] = {
'graphql.type.name' => type.graphql_name,
'graphql.lazy' => true
}.freeze
end
@_otel_lazy_authorized_attrs_cache.compare_by_identity

@_otel_field_attrs_cache = Hash.new do |h, field|
h[field] = {
'graphql.field.parent' => field.owner&.graphql_name,
'graphql.field.name' => field.graphql_name,
'graphql.lazy' => false
}.freeze
end
@_otel_field_attrs_cache.compare_by_identity

@_otel_lazy_field_attrs_cache = Hash.new do |h, field|
h[field] = {
'graphql.field.parent' => field.owner&.graphql_name,
'graphql.field.name' => field.graphql_name,
'graphql.lazy' => true
}.freeze
end
@_otel_lazy_field_attrs_cache.compare_by_identity

@_otel_resolve_type_attrs_cache = Hash.new do |h, type|
h[type] = {
'graphql.type.name' => type.graphql_name,
'graphql.lazy' => false
}.freeze
end
@_otel_resolve_type_attrs_cache.compare_by_identity

@_otel_lazy_resolve_type_attrs_cache = Hash.new do |h, type|
h[type] = {
'graphql.type.name' => type.graphql_name,
'graphql.lazy' => true
}.freeze
end
@_otel_lazy_resolve_type_attrs_cache.compare_by_identity
super
end

Expand Down Expand Up @@ -73,26 +126,18 @@ def execute_query_lazy(query:, multiplex:, &block)

def execute_field(field:, query:, ast_node:, arguments:, object:, &block)
platform_key = _otel_execute_field_key(field: field)
return super unless platform_key
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't explain why, but replacing this super call (and the one below in execute_field_lazy) made up a big part of the memory improvement. In the "before" profile, these lines appear in the result:

       345  /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/graphql-2.1.6/lib/graphql/schema/object.rb:82
       345  /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/opentelemetry-instrumentation-graphql-0.26.7/lib/opentelemetry/instrumentation/graphql/tracers/graphql_trace.rb:104
       344  /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/graphql-2.1.6/lib/graphql/execution/interpreter/runtime.rb:600
       344  /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/opentelemetry-instrumentation-graphql-0.26.7/lib/opentelemetry/instrumentation/graphql/tracers/graphql_trace.rb:76
       343  otel_bench.rb:104

But adding these arguments caused them to not appear anymore.

Looking at the breakdown by class, my only guess is that, somewhere under the hood, Ruby was making a Hash as part of this code:

  allocated objects by class
  -----------------------------------
-     6230  Hash
+     5126  Hash
      2395  Array
-     1188  Proc
+     1194  Proc
-      871  String
+      875  String
       785  Thread::Mutex
       784  OpenTelemetry::Context
       784  OpenTelemetry::SDK::Trace::Samplers::Result
       784  OpenTelemetry::SDK::Trace::Span
       784  OpenTelemetry::Trace::SpanContext
       423  GraphQL::Execution::Lazy
       345  GraphQL::Execution::Interpreter::Runtime::GraphQLResultHash
       301  MySchema::Book

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing this. I wonder if there is a performance Rubocop linter we can leverage here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is the rubocop-performance gem, but after doing a quick test with the current instrumentation, these issues weren't pointed out.

return super(field: field, query: query, ast_node: ast_node, object: object, arguments: arguments, &block) unless platform_key

attributes = {
'graphql.field.parent' => field.owner&.graphql_name,
'graphql.field.name' => field.graphql_name,
'graphql.lazy' => false
}
attributes = @_otel_field_attrs_cache[field]

tracer.in_span(platform_key, attributes: attributes, &block)
end

def execute_field_lazy(field:, query:, ast_node:, arguments:, object:, &block)
platform_key = _otel_execute_field_key(field: field)
return super unless platform_key
return super(field: field, query: query, ast_node: ast_node, object: object, arguments: arguments, &block) unless platform_key

attributes = {
'graphql.field.parent' => field.owner&.graphql_name,
'graphql.field.name' => field.graphql_name,
'graphql.lazy' => true
}
attributes = @_otel_lazy_field_attrs_cache[field]

tracer.in_span(platform_key, attributes: attributes, &block)
end
Expand All @@ -101,10 +146,7 @@ def authorized(query:, type:, object:, &block)
platform_key = @_otel_authorized_key_cache[type]
return super unless platform_key

attributes = {
'graphql.type.name' => type.graphql_name,
'graphql.lazy' => false
}
attributes = @_otel_authorized_attrs_cache[type]

tracer.in_span(platform_key, attributes: attributes, &block)
end
Expand All @@ -113,33 +155,19 @@ def authorized_lazy(query:, type:, object:, &block)
platform_key = @_otel_authorized_key_cache[type]
return super unless platform_key

attributes = {
'graphql.type.name' => type.graphql_name,
'graphql.lazy' => true
}

attributes = @_otel_lazy_authorized_attrs_cache[type]
tracer.in_span(platform_key, attributes: attributes, &block)
end

def resolve_type(query:, type:, object:, &block)
platform_key = @_otel_resolve_type_key_cache[type]

attributes = {
'graphql.type.name' => type.graphql_name,
'graphql.lazy' => false
}

attributes = @_otel_resolve_type_attrs_cache[type]
tracer.in_span(platform_key, attributes: attributes, &block)
end

def resolve_type_lazy(query:, type:, object:, &block)
platform_key = @_otel_resolve_type_key_cache[type]

attributes = {
'graphql.type.name' => type.graphql_name,
'graphql.lazy' => true
}

attributes = @_otel_lazy_resolve_type_attrs_cache[type]
tracer.in_span(platform_key, attributes: attributes, &block)
end

Expand Down