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.

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

Stop byteslicing empty strings in breadcrumbs #2574

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

Conversation

nirebu
Copy link

@nirebu nirebu commented Mar 6, 2025

Please keep these instructions in mind so we can review it more efficiently:

  • Add the references of all the related issues/PRs in the description
  • Whether it's a new feature or a bug fix, make sure they're covered by new test cases
  • If this PR contains any refactoring work, please give it its own commit(s)
  • Finally, please add an entry to the corresponding changelog

Other Notes

  • We squash all commits before merging
  • We generally review new PRs within a week
  • If you have any question, you can ask for feedback in our discord community first

Description

Describe your changes:

If we don't pass a message to the breadcrumb message, we default to an empty string. However we can avoid byteslicing it, which allocates a new empty string every time regardless.

This is a pretty hot path, and the tests pass. Only difference I can think of is returning a frozen empty string, which could cause issues downstream. Let me know if it should be dup-ed!

require 'benchmark-memory'

MAX_MESSAGE_SIZE_IN_BYTES = 1024 * 8

def current(message)
  (message || "").byteslice(0..MAX_MESSAGE_SIZE_IN_BYTES)
end

def updated(message)
  message ? message.byteslice(0..MAX_MESSAGE_SIZE_IN_BYTES) : ""
end

Benchmark.memory do |x|
  x.report("current") do
    current(nil)
  end

  x.report("updated") do
    updated(nil)
  end

  x.compare!
end

Calculating -------------------------------------
current 80.000 memsize ( 0.000 retained)
2.000 objects ( 0.000 retained)
1.000 strings ( 0.000 retained)
updated 0.000 memsize ( 0.000 retained)
0.000 objects ( 0.000 retained)
0.000 strings ( 0.000 retained)

Comparison:
updated: 0 allocated
current: 80 allocated - Infx more

If we don't pass a message to the breadcrumb message, we default to an
empty string. However we can avoid byteslicing it, which allocates a new
empty string every time regardless.

```rb

require 'benchmark-memory'

MAX_MESSAGE_SIZE_IN_BYTES = 1024 * 8

def current(message)
  (message || "").byteslice(0..MAX_MESSAGE_SIZE_IN_BYTES)
end

def updated(message)
  message ? message.byteslice(0..MAX_MESSAGE_SIZE_IN_BYTES) : ""
end

Benchmark.memory do |x|
  x.report("current") do
    current(nil)
  end

  x.report("updated") do
    updated(nil)
  end

  x.compare!
end
```

Calculating -------------------------------------
             current    80.000  memsize (     0.000  retained)
                         2.000  objects (     0.000  retained)
                         1.000  strings (     0.000  retained)
             updated     0.000  memsize (     0.000  retained)
                         0.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)

Comparison:
             updated:          0 allocated
             current:         80 allocated - Infx more
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant