Skip to content

perf: cache longest_load_path and compute_filename results #2904

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

Open
HazAT wants to merge 1 commit into
base: master
Choose a base branch
from
Open

perf: cache longest_load_path and compute_filename results #2904

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
77 changes: 75 additions & 2 deletions sentry-ruby/lib/sentry/interfaces/stacktrace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,77 @@ def inspect
private

# Not actually an interface, but I want to use the same style
# Cache for longest_load_path lookups — shared across all frames
@load_path_cache = {}
@load_path_size = nil
# Cache for compute_filename results — many frames share identical abs_paths
# Separate caches for in_app=true and in_app=false to avoid composite keys
@filename_cache_in_app = {}
@filename_cache_not_in_app = {}
@filename_project_root = nil

class << self
def check_load_path_freshness
current_size = $LOAD_PATH.size
if @load_path_size != current_size
@load_path_cache = {}
@filename_cache_in_app = {}
@filename_cache_not_in_app = {}
@load_path_size = current_size
end
end

def longest_load_path_for(abs_path)
check_load_path_freshness

@load_path_cache.fetch(abs_path) do
result = $LOAD_PATH.select { |path| abs_path.start_with?(path.to_s) }.max_by(&:size)
@load_path_cache[abs_path] = result
end
end

def cached_filename(abs_path, project_root, in_app, strip_backtrace_load_path)
return abs_path unless abs_path
return abs_path unless strip_backtrace_load_path

check_load_path_freshness

# Invalidate filename cache when project_root changes
if @filename_project_root != project_root
@filename_cache_in_app = {}
@filename_cache_not_in_app = {}
@filename_project_root = project_root
end

cache = in_app ? @filename_cache_in_app : @filename_cache_not_in_app
cache.fetch(abs_path) do
under_root = project_root && abs_path.start_with?(project_root)
prefix =
if under_root && in_app
project_root
elsif under_root
longest_load_path_for(abs_path) || project_root
else
longest_load_path_for(abs_path)
end

result = if prefix
prefix_str = prefix.to_s
offset = if prefix_str.end_with?(File::SEPARATOR)
prefix_str.length
else
prefix_str.length + 1
end
abs_path.byteslice(offset, abs_path.bytesize - offset)
Copy link

@cursor cursor bot Mar 18, 2026

Choose a reason for hiding this comment

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

Mixing character length with byte offset in byteslice

Medium Severity

The offset is computed using prefix_str.length (character count) but passed to abs_path.byteslice (which expects a byte offset). The original compute_filename correctly used String#[] (character-based indexing) with String#length. For paths containing multibyte UTF-8 characters (e.g., accented letters in directory names), character count differs from byte count, producing incorrect filenames — typically with a leading / or corrupted characters.

Fix in Cursor Fix in Web

else
Comment on lines +83 to +88
Copy link

@sentry sentry bot Mar 18, 2026

Choose a reason for hiding this comment

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

Bug: The code uses a character count (String#length) as an offset for a byte-based operation (String#byteslice), causing incorrect filename stripping for paths with multibyte characters.
Severity: MEDIUM

Suggested Fix

To ensure the offset is calculated correctly in bytes, replace the use of prefix_str.length with prefix_str.bytesize when determining the offset for the byteslice operation. This will align the offset calculation with the byte-based expectation of the byteslice method.

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.rb#L83-L88

Potential issue: The `cached_filename` method calculates an offset using
`prefix_str.length`, which returns the number of characters. This offset is then used
with `abs_path.byteslice`, which expects an offset in bytes. When a file path contains
multibyte UTF-8 characters, the character count and byte count will not match. This
discrepancy causes `byteslice` to slice the string at an incorrect byte position, which
can lead to a corrupted filename or a `nil` value. This incorrect result may then be
cached, causing subsequent lookups for the same path to also return an incorrect value.

Did we get this right? 👍 / 👎 to inform future reviews.

abs_path
end

cache[abs_path] = result
end
end
end

class Frame < Interface
attr_accessor :abs_path, :context_line, :function, :in_app, :filename,
:lineno, :module, :pre_context, :post_context, :vars
Expand All @@ -36,7 +107,9 @@ def initialize(project_root, line, strip_backtrace_load_path = true)
@lineno = line.number
@in_app = line.in_app
@module = line.module_name if line.module_name
@filename = compute_filename
@filename = StacktraceInterface.cached_filename(
@abs_path, project_root, @in_app, strip_backtrace_load_path
)
end

def to_s
Expand Down Expand Up @@ -82,7 +155,7 @@ def under_project_root?
end

def longest_load_path
$LOAD_PATH.select { |path| abs_path.start_with?(path.to_s) }.max_by(&:size)
StacktraceInterface.longest_load_path_for(abs_path)
end
end
end
Expand Down
Loading