-
Notifications
You must be signed in to change notification settings - Fork 250
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.
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
fix: Reduce allocations on GraphQL hot paths #1544
Conversation
@@ -49,17 +49,18 @@ def valid_value?(value) | |||
end | |||
|
|||
def valid_attributes?(owner, kind, attrs) | |||
attrs.nil? || attrs.all? do |k, v| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't exactly understand it, but this was the top allocating location in my benchmark. By switching to each
(presumably, a simpler implementation under the hood?), the allocation was removed:
allocated objects by location
-----------------------------------
- 1941 /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/opentelemetry-sdk-1.3.1/lib/opentelemetry/sdk/internal.rb:52
1568 /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/opentelemetry-sdk-1.3.1/lib/opentelemetry/sdk/trace/tracer_provider.rb:143
803 /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/graphql-2.1.6/lib/graphql/execution/lazy.rb:21
784 /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/opentelemetry-api-1.2.3/lib/opentelemetry/context.rb:151
784 /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/opentelemetry-api-1.2.3/lib/opentelemetry/context.rb:153
784 /Users/rmosolgo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/opentelemetry-api-1.2.3/lib/opentelemetry/trace.rb:39
Looking at the breakdown by class, I'm guessing that Ruby makes an Array in there, somewhere:
allocated objects by class
-----------------------------------
- 6230 Hash
+ 5453 Hash
- 2395 Array
1188 Proc
- 871 String
+ 875 String
785 Thread::Mutex
784 OpenTelemetry::Context
(The difference in Hash allocations is probably best explained by the other part of this PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weirdly, this is failing CI. I'll have to think through the logic, but we're keeping some Event attributes that we should drop.
@@ -317,7 +317,7 @@ def initialize(context, parent_context, parent_span, name, kind, parent_span_id, | |||
# SpanData. | |||
@monotonic_start_timestamp = monotonic_now | |||
@realtime_start_timestamp = if parent_span.recording? | |||
relative_realtime(parent_span.realtime_start_timestamp, parent_span.monotonic_start_timestamp) | |||
relative_realtime(parent_span.realtime_start_timestamp, parent_span.monotonic_start_timestamp, @monotonic_start_timestamp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, every Span#initialize
was calling Process.clock_gettime
twice: once to initialize @monotonic_start_timestamp
, and another time in the implementation of relative_realtime
. I noticed that, by passing a time in to that method, we might save that second call out to the system function.
Another use of relative_realtime
had to be updated below, it calls monotonic_now
directly and passes the result.
@@ -297,7 +297,7 @@ def initialize(context, parent_context, parent_span, name, kind, parent_span_id, | |||
@total_recorded_events = 0 | |||
@total_recorded_links = links&.size || 0 | |||
@total_recorded_attributes = attributes&.size || 0 | |||
@attributes = attributes.nil? ? nil : Hash[attributes] # We need a mutable copy of attributes. | |||
@attributes = attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes, this line was calling an already-dup
ed Hash, which had been created by attributes.merge(result.attributes)
. Instead of duping it here, I added a .dup
call to the calling code in tracer_provider.rb
. That way, we'd only create new Hashes when we really needed them.
I didn't find any other callers to this class -- was that a safe substitution?
@@ -141,7 +141,7 @@ def internal_start_span(name, kind, attributes, links, start_timestamp, parent_c | |||
if result.recording? && !@stopped | |||
trace_flags = result.sampled? ? OpenTelemetry::Trace::TraceFlags::SAMPLED : OpenTelemetry::Trace::TraceFlags::DEFAULT | |||
context = OpenTelemetry::Trace::SpanContext.new(trace_id: trace_id, span_id: span_id, trace_flags: trace_flags, tracestate: result.tracestate) | |||
attributes = attributes&.merge(result.attributes) || result.attributes | |||
attributes = attributes&.merge(result.attributes) || result.attributes.dup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, this hash was duped inside Span.new
(described above), but it isn't anymore, so it's dup'ed here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking while we debug the CI failures.
Co-authored-by: Francis Bogsanyi <[email protected]>
Co-authored-by: Francis Bogsanyi <[email protected]>
Good call on |
👋 I was investigating memory consumption from a GraphQL perspective and I found a few places I thought might be improved. In my benchmark, this improved the total memory consumption by 11% (17% of objects):
otel_bench.rb