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

Reduce SmolStr <-> &str conversions and copies #6688

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

Conversation

milianw
Copy link
Contributor

@milianw milianw commented Oct 31, 2024

SmolStr has an Arc internally for large strings. This allows cheap copies of large strings, but we lose that ability when we convert the SmolStr to a &str and then reconstruct a SmolStr from that slice.

I was hoping for some larger gains here, considering the impact of this code change, but it only removes ~50k allocations, while the impact on the runtime is not noticeable at all.

Still, I believe this is the right thing to do.

Before:

        allocations:            2338981

  Time (mean ± σ):     988.3 ms ±  17.9 ms    [User: 690.2 ms, System: 206.4 ms]
  Range (min … max):   956.4 ms … 1016.3 ms    10 runs

After:

        allocations:            2287723

  Time (mean ± σ):     989.8 ms ±  23.2 ms    [User: 699.2 ms, System: 197.6 ms]
  Range (min … max):   945.3 ms … 1021.4 ms    10 runs

SmolStr has an Arc internally for large strings. This allows
cheap copies of large strings, but we lose that ability
when we convert the SmolStr to a &str and then reconstruct a
SmolStr from that slice.

I was hoping for some larger gains here, considering the impact
of this code change, but it only removes ~50k allocations,
while the impact on the runtime is not noticeable at all.

Still, I believe this is the right thing to do.

Before:
```
        allocations:            2338981

  Time (mean ± σ):     988.3 ms ±  17.9 ms    [User: 690.2 ms, System: 206.4 ms]
  Range (min … max):   956.4 ms … 1016.3 ms    10 runs
```

After:
```
        allocations:            2287723

  Time (mean ± σ):     989.8 ms ±  23.2 ms    [User: 699.2 ms, System: 197.6 ms]
  Range (min … max):   945.3 ms … 1021.4 ms    10 runs
```
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