perf: cache Frame objects in StacktraceBuilder by Line identity#2906
perf: cache Frame objects in StacktraceBuilder by Line identity#2906
Conversation
⚠️ Needs closer review — caches Frame objects by Line object_id. When combined with Line object caching (where identical backtrace lines return the same Line instance), this cache avoids recreating Frame objects for the same source locations across exceptions. The cache key is Line#object_id, which is stable for cached Line objects. For non-cached Lines, unique object_ids mean no false cache hits. Cache is per-StacktraceBuilder instance (not class-level), bounded to 2048 entries. Frame objects are effectively read-only after context is set — attributes are only read during to_h serialization. This is most effective when paired with the Line object caching PR, but provides modest benefit independently (avoids Frame recreation within a single exception's backtrace when duplicate frames exist).
Reduce total allocated memory from 442k to 206k bytes (-53.5%) and
objects from 3305 to 1538 (-53.5%) per Rails exception capture.
All changes are internal optimizations with zero behavior changes.
Key optimizations:
- Cache longest_load_path and compute_filename results (class-level,
invalidated on $LOAD_PATH changes)
- Cache backtrace line parsing and Line/Frame object creation (bounded
at 2048 entries)
- Optimize LineCache with Hash#fetch, direct context setting, and
per-(filename, lineno) caching
- Avoid unnecessary allocations: indexed regex captures, match? instead
of =~, byteslice, single-pass iteration in StacktraceBuilder
- RequestInterface: avoid env.dup, cache header name transforms, ASCII
fast-path for encoding
- Scope/BreadcrumbBuffer: shallow dup instead of deep_dup where inner
values are not mutated after duplication
- Hub#add_breadcrumb: hint default nil instead of {} to avoid empty
hash allocation
See sub-PRs for detailed review by risk level:
- #2902 (low risk) — hot path allocation avoidance
- #2903 (low risk) — LineCache optimization
- #2904 (medium risk) — load path and filename caching
- #2905 (needs review) — backtrace parse caching
- #2906 (needs review) — Frame object caching
- #2907 (needs review) — Scope/BreadcrumbBuffer shallow dup
- #2908 (medium risk) — RequestInterface optimizations
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| def convert_parsed_line_into_frame(line) | ||
| # Cache frames by Line object identity — same Line produces same Frame | ||
| cache_key = line.object_id |
There was a problem hiding this comment.
Bug: The frame cache uses line.object_id as a key, which is unsafe as object_ids can be reused after garbage collection, leading to incorrect cache hits.
Severity: HIGH
Suggested Fix
Use a cache key that uniquely and reliably identifies the source line, such as a string combining file, lineno, and method. This avoids the problem of object_id reuse. Additionally, consider cloning Frame objects when returning them from the cache to prevent mutations in callbacks from affecting the cached instance.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentry-ruby/lib/sentry/interfaces/stacktrace_builder.rb#L82
Potential issue: The frame caching mechanism uses `line.object_id` as a cache key. Since
Ruby can reuse `object_id`s for new objects after the original object has been garbage
collected, this can lead to cache key collisions. A `Frame` object for one source code
location could be incorrectly returned for a completely different location if a new
`Line` object happens to be assigned a recycled `object_id`. This would result in
corrupted stack trace data, where frames from one exception's backtrace appear in
another's.
Did we get this right? 👍 / 👎 to inform future reviews.
| cached_frame = @frame_cache&.[](cache_key) | ||
| return cached_frame if cached_frame |
There was a problem hiding this comment.
Bug: Cached Frame objects are mutable and can be modified by callbacks. These mutations persist in the cache, causing data to leak between different events.
Severity: MEDIUM
Suggested Fix
Clone the Frame object before returning it from the cache. This ensures that any mutations made by callbacks affect only a copy of the object, leaving the cached version pristine for subsequent use. For example, change return cached_frame to return cached_frame.dup.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentry-ruby/lib/sentry/interfaces/stacktrace_builder.rb#L83-L84
Potential issue: The `StacktraceBuilder` caches and reuses `Frame` object instances.
These objects are passed to a user-configurable `frame_callback`, which can mutate them
(e.g., by adding `vars`). Because the cache stores and returns a reference to the same
mutable object, any modifications made by the callback persist in the cache. Subsequent
exceptions that hit the same cache entry will receive the already-mutated `Frame`
object, leading to data from one event leaking into another.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| frame.set_context(linecache, context_lines) if context_lines | ||
|
|
||
| @frame_cache ||= {} | ||
| @frame_cache[cache_key] = frame if @frame_cache.size < 2048 |
There was a problem hiding this comment.
Cached frames mutated after caching cause data leakage
High Severity
StacktraceBuilder is a memoized singleton on Configuration, so @frame_cache persists across all events for the application lifetime. Cached Frame objects are mutated after caching — SingleExceptionInterface.build_with_stacktrace sets stacktrace.frames.last&.vars = locals, and frame_callback can also modify frames in-place (as demonstrated in the existing test). These mutations corrupt the cached Frame, causing exception locals or callback modifications from one error event to bleed into future events that reuse the same cached frame.
| frame.set_context(linecache, context_lines) if context_lines | ||
|
|
||
| @frame_cache ||= {} | ||
| @frame_cache[cache_key] = frame if @frame_cache.size < 2048 |
There was a problem hiding this comment.
Cache keyed by object_id breaks after GC
Medium Severity
The cache keys by line.object_id but stores only the integer key and Frame value — it holds no reference to the Line object itself. Since StacktraceBuilder is a long-lived singleton, original Line objects can be garbage collected and their object_id reassigned to completely different Line objects. This produces false cache hits, returning an incorrect Frame for a different backtrace line.
1 participant
Part of #2901 (reduce memory allocations by ~53%)
Changes
Cache Frame objects in
StacktraceBuilder#convert_parsed_line_into_framekeyed byLine#object_id. When combined with Line object caching (#2905), the same cached Line instances produce the same Frame instances, eliminating ~72 duplicate Frame creations per exception.StacktraceBuilderinstance (not class-level)to_hserializationReview focus
object_id— works correctly when Line objects are cached (same object = same id). For non-cached Lines, unique ids mean no false cache hits. Is this assumption documented clearly enough?