Add repeat for Char to fix performance of pow #22797

musm · 2017-07-13T19:17:25Z

But more importantly fix the bug for ::Char ^ 0 and ::Char^1

As a future optimization someone may want to improve the nonascii case to remove the allocation for the string conversion.

# benchmark using 0.6

julia> using BenchmarkTools; import Base.repeat, Base._string_n

julia> function repeat(c::Char, r::Integer)
           r < 0 && throw(ArgumentError("can't repeat a character $r times"))
           if isascii(c)
               out = _string_n(r)
               ccall(:memset, Ptr{Void}, (Ptr{UInt8}, Cint, Csize_t), out, c, r)
               return out
           else
               return repeat(string(c), r)
           end
       end
repeat (generic function with 4 methods)

julia> @btime repeat($('x'), $(100));
  28.140 ns (1 allocation: 128 bytes)

julia> @btime repeat($("x"), $(100));
  280.596 ns (1 allocation: 128 bytes)

julia> @btime repeat($('x'), $(10));
  18.158 ns (1 allocation: 32 bytes)

julia> @btime repeat($("x"), $(10));
  42.239 ns (1 allocation: 32 bytes)

julia> @btime repeat($('x'), $(2));
  18.158 ns (1 allocation: 32 bytes)

julia> @btime repeat($("x"), $(2));
  21.711 ns (1 allocation: 32 bytes)

tkelman · 2017-07-13T19:23:40Z

base/strings/string.jl

+"""
+    repeat(s::String, r::Integer)
+
+Repeat a string `r` times.


little odd that repeat is currently in the linalg doc index, maybe should be moved to more general collections section

We could have it in both the linalg and string indices, with AbstractArray and AbstractString signatures.

But I'm not sure why anyone would ever call repeat for a string rather than using ^.

Updated PR please check if this now looks good.

the multiple docstrings would need to be distinguished by signature otherwise they will all be repeated in both sections

@tkelman thanks will update. This means I should include:

Base.repeat(::AbstractString, ::Integer) Base.repeat(::Char, ::Integer)

well those two docstrings would probably be fine to list together since they're closely related - it's the array one that should be in a different section

stevengj · 2017-07-13T21:15:55Z

base/strings/string.jl

@@ -438,3 +449,25 @@ function repeat(s::String, r::Integer)
    end
    return out
 end
+
+"""
+    repeat(c::Char, r::Integer) -> String


Why don't we use Julia syntax repeat(c::Char, r::Integer)::String for this sort of thing?

Agreed, that change should however be made or brought up in another PR/issue.

I couldn't find an open issue on this, so I filed one here #22804

stevengj · 2017-07-13T21:16:51Z

base/strings/string.jl

+```
+"""
+function repeat(c::Char, r::Integer)
+    r < 0 && throw(ArgumentError("can't repeat a character $r times"))


Put the r < 0 check inside the isascii check, since repeat(string(c), r) also does this check; otherwise you are doing the same check twice for non-ascii chars.

stevengj · 2017-07-13T21:18:45Z

test/strings/basic.jl

-@test repeat("xx",3) == repeat("x",6) == "xxxxxx"
-@test repeat("αα",3) == repeat("α",6) == "αααααα"
+@test repeat("xx",3) == repeat("x",6) == repeat('x',6) == "xxxxxx"
+@test repeat("αα",3) == repeat("α",6) == repeat('α',6) == "αααααα"


I would also add a regression test for ^ to make sure 'x'^0 == "" and 'x'^1 == "x".

Thanks done.

fredrikekre · 2017-07-13T21:37:23Z

base/strings/types.jl


-Repeat `n` times the string `s`.
+Repeat `n` times the string or character `s`.


Almost reads like "repeat n * string". Perhaps:
Repeat the string or character `s` `n` times.

musm · 2017-07-13T21:56:30Z

Interestingly the performance characteristics of repeat on master have changed substantially than from 0.6.

stevengj · 2017-07-13T22:09:55Z

test/strings/basic.jl

+@test repeat("x",1) == repeat('x',1) == "x"
+@test repeat("x",0) == repeat('x',0) == ""
+@test "x"^1 == 'x'^1 == "x"
+@test "x"^0 == 'x'^0 == ""


Probably merge these two tests with the previous lines, e.g. @test "x"^1 == 'x'^1 == repeat("x",1) == repeat('x',1) == "x"

musm · 2017-07-14T03:48:01Z

Updated to also test AbstractString subtypes

stevengj · 2017-07-14T13:07:19Z

base/strings/types.jl

+"""
+    repeat(s::AbstractString, r::Integer)
+
+Repeat a string `r` times.


Probably the docstring should also cross-reference ^; similarly for Char.

musm · 2017-07-14T14:18:33Z

@tkelman and @stevengj I have addressed your reviews

(please, don't forget to squash merge)

stevengj · 2017-07-14T14:34:26Z

base/strings/types.jl

@@ -155,7 +155,7 @@ reverseind(s::SubString{String}, i::Integer) =
 """
    repeat(s::AbstractString, r::Integer)

-Repeat a string `r` times.
+Repeat a string `r` times. This can equivalently be accomplished by calling [`^`](@ref).


Would be clearer to write:

by calling [`s^r`](@ref ^).

Ideally, we should cross-reference ^(::AbstractString, ::Integer) ... @MichaelHatherly, does @ref ^(::AbstractString, ::Integer) work in Documenter?

Thanks, fixed.

stevengj · 2017-07-15T13:12:16Z

(Travis failure is #17626. Not sure what the ProcessExitedException on AppVeyor is about.)

musm · 2017-07-15T19:02:03Z

I think there is a bug in the implementation here
e.g. try this on $ , which spits

"\$\$\$\$\$\$\$\$\$\$\$\$\$\$\$\$\$\$\$\$"

TotalVerb · 2017-07-15T20:28:02Z

@musm That's because $ must be escaped within double quotes.

musm · 2017-07-15T22:11:20Z

@TotalVerb oh right, thanks!

I added a new commit which removed the GenericString test by accident, I dropped the commit.

KristofferC · 2017-07-16T04:58:57Z

Good to go then?

musm · 2017-07-16T04:59:42Z

yes (please squash)

* Add repeat for Char to fix performance of pow * Address reviews * Update docstring

ararslan requested a review from stevengj July 13, 2017 19:17

tkelman reviewed Jul 13, 2017

View reviewed changes

musm force-pushed the ch branch from f37688b to bc7bbb0 Compare July 13, 2017 19:24

stevengj reviewed Jul 13, 2017

View reviewed changes

musm force-pushed the ch branch from bc7bbb0 to b5fa43c Compare July 13, 2017 21:37

fredrikekre reviewed Jul 13, 2017

View reviewed changes

stevengj reviewed Jul 13, 2017

View reviewed changes

musm force-pushed the ch branch from b5fa43c to 5dd48dc Compare July 13, 2017 22:19

Add repeat for Char to fix performance of pow

1aae4ed

musm force-pushed the ch branch from 5dd48dc to 1aae4ed Compare July 13, 2017 22:50

stevengj reviewed Jul 14, 2017

View reviewed changes

Address reviews

bf135f6

stevengj reviewed Jul 14, 2017

View reviewed changes

Update docstring

467738c

stevengj approved these changes Jul 15, 2017

View reviewed changes

musm force-pushed the ch branch from 6e132ae to 467738c Compare July 15, 2017 22:10

musm mentioned this pull request Jul 15, 2017

Remove special cases for AbstractString repeat #22805

Merged

KristofferC merged commit c2a848a into JuliaLang:master Jul 16, 2017

musm deleted the ch branch July 16, 2017 05:07

jeffwong pushed a commit to jeffwong/julia that referenced this pull request Jul 24, 2017

Add repeat for Char to fix performance of pow (JuliaLang#22797)

67f93bd

* Add repeat for Char to fix performance of pow * Address reviews * Update docstring

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add repeat for Char to fix performance of pow #22797

Add repeat for Char to fix performance of pow #22797

musm commented Jul 13, 2017 •

edited

Loading

tkelman Jul 13, 2017

stevengj Jul 13, 2017

stevengj Jul 13, 2017

musm Jul 13, 2017

tkelman Jul 13, 2017

musm Jul 13, 2017 •

edited

Loading

tkelman Jul 14, 2017

stevengj Jul 13, 2017

musm Jul 13, 2017

musm Jul 14, 2017

stevengj Jul 13, 2017

stevengj Jul 13, 2017 •

edited

Loading

musm Jul 13, 2017

fredrikekre Jul 13, 2017

musm commented Jul 13, 2017

stevengj Jul 13, 2017

musm commented Jul 14, 2017

stevengj Jul 14, 2017

musm commented Jul 14, 2017

stevengj Jul 14, 2017

musm Jul 14, 2017

stevengj commented Jul 15, 2017 •

edited

Loading

musm commented Jul 15, 2017 •

edited

Loading

TotalVerb commented Jul 15, 2017

musm commented Jul 15, 2017

KristofferC commented Jul 16, 2017

musm commented Jul 16, 2017 •

edited

Loading


		Repeat `n` times the string `s`.
		Repeat `n` times the string or character `s`.

Add repeat for Char to fix performance of pow #22797

Add repeat for Char to fix performance of pow #22797

Conversation

musm commented Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm commented Jul 13, 2017

Choose a reason for hiding this comment

musm commented Jul 14, 2017

Choose a reason for hiding this comment

musm commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Jul 15, 2017 • edited Loading

musm commented Jul 15, 2017 • edited Loading

TotalVerb commented Jul 15, 2017

musm commented Jul 15, 2017

KristofferC commented Jul 16, 2017

musm commented Jul 16, 2017 • edited Loading

musm commented Jul 13, 2017 •

edited

Loading

musm Jul 13, 2017 •

edited

Loading

stevengj Jul 13, 2017 •

edited

Loading

stevengj commented Jul 15, 2017 •

edited

Loading

musm commented Jul 15, 2017 •

edited

Loading

musm commented Jul 16, 2017 •

edited

Loading