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.

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

Jump to bottom

Use StringMemory instead of StringVector where possible #53962

Merged
merged 2 commits into from
Apr 5, 2024
Merged

Use StringMemory instead of StringVector where possible #53962

merged 2 commits into from
Apr 5, 2024

Conversation

Zentrik
Copy link
Member

@Zentrik Zentrik commented Apr 5, 2024 •
edited
Loading

On 1.11.0-alpha2
Old:

@benchmark Base.dec($0x1, $0, $false)
BenchmarkTools.Trial: 10000 samples with 994 evaluations.
 Range (min … max):  33.702 ns …   4.242 μs  ┊ GC (min … max):  0.00% … 97.61%
 Time  (median):     37.626 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   45.787 ns ± 147.794 ns  ┊ GC (mean ± σ):  14.53% ±  4.47%

    ▄▅▆▇█▇▇▅▃▃▂▂▂▁    ▁▂▁▁▁             ▁▁   ▁                 ▂
  ▄▇███████████████▇▇██████▇█▆▆▄▄▃▄▅▄▆▇████████▆▅▅▇▆▅▆▄▄▅▄▄▄▁▅ █
  33.7 ns       Histogram: log(frequency) by time      67.5 ns <

 Memory estimate: 88 bytes, allocs estimate: 3.

New:

BenchmarkTools.Trial: 10000 samples with 995 evaluations.
 Range (min … max):  27.538 ns …   3.397 μs  ┊ GC (min … max):  0.00% … 97.86%
 Time  (median):     30.151 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   34.547 ns ± 105.101 ns  ┊ GC (mean ± σ):  10.37% ±  3.39%

       ▁ █▆▃  ▁
  ▂▂▃▃▅█████▆████▆▄▄▃▃▃▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▂▂▂▂▁▂▂▂▂▂▂▂▂▂▂▂▂▂ ▃
  27.5 ns         Histogram: frequency by time         43.8 ns <

 Memory estimate: 56 bytes, allocs estimate: 2.

Fixes #53950, actually now even faster than 1.10.2.

It doesn't look like the length is ever changed and we don't return these StringMemorys so this change should be fine.

@Zentrik
Use StringMemory to convert integer to string ~20% faster
6d4b192 
On `1.11.0-alpha2`
Old:
```julia
@benchmark Base.dec($0x1, $0, $false)
BenchmarkTools.Trial: 10000 samples with 994 evaluations.
 Range (min … max):  33.702 ns …   4.242 μs  ┊ GC (min … max):  0.00% … 97.61%
 Time  (median):     37.626 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   45.787 ns ± 147.794 ns  ┊ GC (mean ± σ):  14.53% ±  4.47%

    ▄▅▆▇█▇▇▅▃▃▂▂▂▁    ▁▂▁▁▁             ▁▁   ▁                 ▂
  ▄▇███████████████▇▇██████▇█▆▆▄▄▃▄▅▄▆▇████████▆▅▅▇▆▅▆▄▄▅▄▄▄▁▅ █
  33.7 ns       Histogram: log(frequency) by time      67.5 ns <

 Memory estimate: 88 bytes, allocs estimate: 3.
```
New:
```julia
BenchmarkTools.Trial: 10000 samples with 995 evaluations.
 Range (min … max):  27.538 ns …   3.397 μs  ┊ GC (min … max):  0.00% … 97.86%
 Time  (median):     30.151 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   34.547 ns ± 105.101 ns  ┊ GC (mean ± σ):  10.37% ±  3.39%

       ▁ █▆▃  ▁
  ▂▂▃▃▅█████▆████▆▄▄▃▃▃▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▂▂▂▂▁▂▂▂▂▂▂▂▂▂▂▂▂▂ ▃
  27.5 ns         Histogram: frequency by time         43.8 ns <

 Memory estimate: 56 bytes, allocs estimate: 2.
```


Fixes JuliaLang#53950, actually now even faster than `1.10.2`.
@Zentrik
Convert some more StringVectors to StringMemory
4b78a9d
@Zentrik Zentrik changed the title Use StringMemory to convert integer to string ~20% faster Use StringMemory instead of StringVector where possible Apr 5, 2024
@Zentrik Zentrik added performance Must go faster strings "Strings!" labels Apr 5, 2024
@giordano giordano added the backport 1.11 Change should be backported to release-1.11 label Apr 5, 2024
@giordano giordano requested a review from vtjnash April 5, 2024 11:57
@oscardssmith oscardssmith merged commit 0e59c9e into JuliaLang:master Apr 5, 2024
13 checks passed
KristofferC pushed a commit that referenced this pull request Apr 9, 2024
@Zentrik
Use StringMemory instead of StringVector where possible (#53962)
df60193 
On `1.11.0-alpha2`
Old:
```julia
@benchmark Base.dec($0x1, $0, $false)
BenchmarkTools.Trial: 10000 samples with 994 evaluations.
 Range (min … max):  33.702 ns …   4.242 μs  ┊ GC (min … max):  0.00% … 97.61%
 Time  (median):     37.626 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   45.787 ns ± 147.794 ns  ┊ GC (mean ± σ):  14.53% ±  4.47%

    ▄▅▆▇█▇▇▅▃▃▂▂▂▁    ▁▂▁▁▁             ▁▁   ▁                 ▂
  ▄▇███████████████▇▇██████▇█▆▆▄▄▃▄▅▄▆▇████████▆▅▅▇▆▅▆▄▄▅▄▄▄▁▅ █
  33.7 ns       Histogram: log(frequency) by time      67.5 ns <

 Memory estimate: 88 bytes, allocs estimate: 3.
```
New:
```julia
BenchmarkTools.Trial: 10000 samples with 995 evaluations.
 Range (min … max):  27.538 ns …   3.397 μs  ┊ GC (min … max):  0.00% … 97.86%
 Time  (median):     30.151 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   34.547 ns ± 105.101 ns  ┊ GC (mean ± σ):  10.37% ±  3.39%

       ▁ █▆▃  ▁
  ▂▂▃▃▅█████▆████▆▄▄▃▃▃▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▂▂▂▂▁▂▂▂▂▂▂▂▂▂▂▂▂▂ ▃
  27.5 ns         Histogram: frequency by time         43.8 ns <

 Memory estimate: 56 bytes, allocs estimate: 2.
```

Fixes #53950, actually now even faster than `1.10.2`.

It doesn't look like the length is ever changed and we don't return
these `StringMemory`s so this change should be fine.

(cherry picked from commit 0e59c9e)
@KristofferC KristofferC mentioned this pull request Apr 9, 2024
Merged
41 tasks
KristofferC added a commit that referenced this pull request Apr 9, 2024
@KristofferC
Backports release 1.11 (#53790)
911b8cf 
Backported PRs:
- [x] #53757 <!-- Add an IndexStyle example to the diagind docstring -->
- [x] #53809 <!-- Add missing GC_POP() in emit_cfunction -->
- [x] #53789 <!-- also check that UUID of project is non-null when
treating it as a package -->
- [x] #53805 <!-- precompilepkgs: simplify custom config printing if
only one -->
- [x] #53822 <!-- Bump libuv -->
- [x] #53837 <!-- update MPFR to 4.2.1 -->
- [x] #53862 <!-- precompilepkgs: fix error reporting -->
- [x] #53774 <!-- Remove some duplicates from emitted compilation traces
-->
- [ ] #53696 <!-- add invokelatest to on_done callback in bracketed
paste -->
- [x] #53383 <!-- Add `_unsetindex!` methods for `SubArray`s and
`CartesianIndex`es -->
- [x] #53475 <!-- Fix boundscheck in unsetindex for SubArrays -->
- [x] #53888 
- [x] #53870 <!-- Revert change to checksum for llvm-julia -->
- [x] #53906 <!-- Add `Base.isrelocatable(pkg)` -->
- [x] #53833 <!-- Profile: make heap snapshots viewable in vscode viewer
-->
- [x] #53961 <!-- `LazyString` in `LinearAlgebra.checksquare` error
message -->
- [x] #53962 <!-- Use StringMemory instead of StringVector where
possible -->
- [x] #53825 <!-- profile: doc: update the `Allocs.@profile` doc string
-->
- [x] #53975 <!-- `LazyString` in `DimensionMismatch` error messages in
broadcasting -->
- [x] #53905 <!-- Avoid repeated precompilation when loading from
non-relocatable cachefiles -->
- [x] #53896 <!-- Make reshape and view on Memory produce Arrays and
delete wrap -->
- [x] #53991 <!-- Test and fix non-int-length bug in `view(::Memory,
::Union{UnitRange, Base.OneTo})` -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Apr 17, 2024
@nhz2 nhz2 mentioned this pull request May 6, 2024
Open
2 tasks
@nhz2 nhz2 mentioned this pull request May 13, 2024
Merged
vtjnash pushed a commit that referenced this pull request Oct 25, 2024
@nhz2
Make String(::Memory) copy (#54457)
2b3a0f0 
A more targeted fix of #54369 than #54372

Preserves the performance improvements added in #53962 by creating a new
internal `_unsafe_takestring!(v::Memory{UInt8})` function that does what
`String(::Memory{UInt8})` used to do.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscardssmith oscardssmith oscardssmith approved these changes

@quinnj quinnj quinnj approved these changes

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees
No one assigned
Labels
performance Must go faster strings "Strings!"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

30% regression in creating string from integer from 1.10.2 to 1.11
5 participants
@Zentrik @quinnj @oscardssmith @giordano @KristofferC