Use Stateful in String code #25736

Keno · 2018-01-24T21:08:16Z

Builds on top of #25731 to remove a couple of explicit uses of the iteration protocol from the string code. Currently upsets codegen, but I figured I'd put it up early anyway to show usage examples for #25731.

StefanKarpinski

Well, this is certainly a showcase for the expressive power of this iterator. Damn! The combination of zip with all to write generic string matching code is really impressive.

StefanKarpinski · 2018-01-24T21:48:54Z

base/iterators.jl

+7
+````
+"""
+struct Stateful{VS,T}


Wouldn't it make more sense for T to come before VS? Or are there cases where you want to dispatch on the iterator state type but not the iterator type itself?

StefanKarpinski · 2018-01-24T21:49:47Z

base/iterators.jl

+    Stateful{VS, T}(itr, Ref{Union{VS, Nothing}}(vs), Ref{Int}(0))
+end
+
+function Stateful{VS}(itr::T) where {VS,T}


Oh, I see. I guess this is where having VS first is handy?

Yes, as I mentioned in the Stateful PR, the iterator assumes that the type of the val/state tuple won't change from the first one. You might want to use Stateful{Any} if you can't guarantee that.

StefanKarpinski · 2018-01-24T21:51:05Z

base/strings/basic.jl

-        c, i = next(a, i)
-        d, j = next(b, j)
+    a, b = Iterators.Stateful(a), Iterators.Stateful(b)
+    for (c, d) in zip(a, b)


Ooo, this is really nice.

StefanKarpinski · 2018-01-24T21:54:03Z

base/strings/basic.jl

        c ≠ d && return ifelse(c < d, -1, 1)
    end
-    return ifelse(done(b, j), 0, -1)
+    isempty(a) && return ifelse(isempty(b), 0, 1)
+    return -1


Maybe write this as isempty(b) - isempty(a)?

StefanKarpinski · 2018-01-24T21:58:10Z

base/strings/io.jl

        if c in esc
            print(io, '\\', c)
        elseif isascii(c)
-            c == '\0'          ? print(io, escape_nul(s,j)) :
+            c == '\0'          ? print(io, escape_nul(peek(a))) :


Beware: implementation of peek is very spotty across different IO types. We should probably fix that.

Yes, this only calls, peek on Stateful though, not on the IO object.

Ah, right. That's fine then.

StefanKarpinski · 2018-01-24T21:59:43Z

base/strings/search.jl

-            return i
+        a = Iterators.Stateful(trest)
+        matched = all(zip(SubString(s, ii), a)) do (c,d)
+            c == d


😻 I would define eq((a,b)) = a == b and then write this:

matched = all(eq, zip(SubString(s, ii), a))

StefanKarpinski · 2018-01-24T22:02:11Z

base/strings/util.jl

-        c, i = next(a,i)
-        d, j = next(b,j)
+    a, b = Iterators.Stateful(a), Iterators.Stateful(b)
+    for (c, d) in zip(a, b)
        (c != d) && (return false)
    end


And then

all(eq, zip(a, b)) && isempty(b)

StefanKarpinski · 2018-01-24T22:02:40Z

base/strings/util.jl

        (c != d) && (return false)
-        i = prevind(a,i)
-        j = prevind(b,j)
    end


And

all(eq, zip(a, b)) && isempty(b)

Keno · 2018-01-26T20:28:58Z

The codegen issues were #25752. This passes tests for me locally now. Need to finish up the Stateful PR and make sure performance doesn't regress here, but then I think this is good to go.

Keno · 2018-01-26T20:29:15Z

@nanosoldier runbenchmarks(ALL, vs=":master")

vtjnash · 2018-01-26T22:37:57Z

base/strings/search.jl

@@ -106,12 +106,10 @@ function findnext(testf::Function, s::AbstractString, i::Integer)
    z = ncodeunits(s) + 1
    1 ≤ i ≤ z || throw(BoundsError(s, i))
    @inbounds i == z || isvalid(s, i) || string_index_err(s, i)
-    while !done(s,i)
-        d, j = next(s,i)
+    for (j, d) in pairs(s[i:end])


enumerate(SubString(s, i))

Not the same thing. This needs string indices not ordinals.

Should probably use a SubString though to avoid copying, no?

I was under the impression that it did (because it does for AbstractString), but turns out I'm wrong for String, so yeah, needs to be SubString.

vtjnash

Looks useful. A few minor fixes. Remove WIP from title?

vtjnash · 2018-01-26T22:39:01Z

base/strings/search.jl

-        end
+        ii = nextind(s, i)
+        a = Iterators.Stateful(trest)
+        matched = all(splat(==), zip(s[ii:end], a))


SubString(z, ii)

Same thing, indexing into a string gives a SubString

Never mind, I'm wrong about this.

vtjnash · 2018-01-26T22:39:22Z

base/strings/search.jl

-            end
+        a = Iterators.Stateful(trest)
+        ii = prevind(s, i)
+        matched = all(zip(Iterators.reverse(pairs(s[1:ii])), a)) do ((idx, c), d)


vtjnash · 2018-01-26T22:40:45Z

base/strings/search.jl

+        a = Iterators.Stateful(trest)
+        ii = prevind(s, i)
+        matched = all(zip(Iterators.reverse(pairs(s[1:ii])), a)) do ((idx, c), d)
+            i = idx


The compiler can't optimize this (a do block closure). Also hard to read.

fair enough, I'll change it to explicitly take the next iteration and compute nextind.

vtjnash · 2018-01-26T22:43:21Z

base/strings/util.jl

-        j = prevind(b,j)
-    end
-    j < b1
+    a, b = Iterators.Stateful.(Iterators.reverse.((a,b)))


The goal isn't to write exceptionally clever code - this doesn't need to use broadcast, it can just be explicit

The goal isn't to write exceptionally clever code

but if it were, I feel like I'd be winning with this PR. I'll change it back.

vtjnash · 2018-01-26T22:45:20Z

base/essentials.jl

@@ -765,3 +765,6 @@ Indicate whether `x` is [`missing`](@ref).
 """
 ismissing(::Any) = false
 ismissing(::Missing) = true
+
+function take! end
+function peek end


editor is forgetting to put trailing newlines?

nanosoldier · 2018-01-27T02:00:39Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Keno · 2018-01-27T21:00:59Z

Rebased to the latest version of the Stateful PR. Let's see

Keno · 2018-01-27T21:01:18Z

@nanosoldier runbenchmarks(ALL, vs=":master")

StefanKarpinski · 2018-01-27T21:24:39Z

base/iterators.jl

+            fixpoint_iter_type(itrT,
+                fieldtype(nextvalstate, 1),
+                fieldtype(nextvalstate, 2)))
+    end


StefanKarpinski · 2018-01-27T21:32:08Z

base/iterators.jl

@@ -441,6 +442,31 @@ julia> collect(Iterators.rest([1,2,3,4], 2))
 """
 rest(itr,state) = Rest(itr,state)

+"""
+    peel(iter)


Cute name, but I wonder if this shouldn't be called headtail or something more explicit like that. Is "peel" a traditional name for this kind of iterator?

https://en.wikipedia.org/wiki/Loop_splitting#Loop_peeling standard among compiler people I guess?

Maybe call it henchmen then?

Maybe, but that term has always meant because of type stability for me, while this is just a generic iteration peel.

Was joking, of course. But I still think headtail might be better.

I guess it's not exported though so it doesn't matter that much at this point.

Keno · 2018-01-27T21:54:22Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-01-28T02:41:38Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

nanosoldier · 2018-01-28T03:34:03Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Many cases can now be handled by the new `Stateful` iteration wrapper, other can handled by `pairs(SubString)`. The latter could use some specialized code to avoid decoding UTF-8 twice, but that's not done in this PR.

Keno · 2018-01-30T15:52:02Z

I've rebased this on top of the tuning I did on master. Hopefully that should limit the performance regressions here. I'm planning to merge this even if there are minor regressions, since it's a prerequisite for the iteration stuff, which is on the 1.0 critical path. The only remaining issue with the stateful iterator I know of is that is sometimes doesn't get allocation elided. That's a decently straightforward optimization that we should be able to do at any time.

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2018-01-30T21:10:30Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

Keno · 2018-01-30T21:21:37Z

The 15% mem alloc regression is real and due to the fact that the Stateful iterator doesn't get elided properly in that case. Either fixing the getfield elim pass or stack allocating values with references would be able to fix that. I'll work on that after 0.7-alpha, but for now I'm gonna merge this to make progress on iterate.

timholy · 2020-08-17T10:46:55Z

As we've shifted to inferring less aggressively, Stateful is starting to turn up as a frequent trouble spot due to

julia> code_typed(Iterators.Stateful, (AbstractString,))
1-element Vector{Any}:
 CodeInfo(
1 ─ %1 = invoke Base.Iterators.approx_iter_type($(Expr(:static_parameter, 1))::Type)::Type
│   %2 = Core.apply_type(Base.Iterators.Stateful, $(Expr(:static_parameter, 1)), %1)::Type{Base.Iterators.Stateful{_A,_B}} where _B where _A
│   %3 = Base.convert($(Expr(:static_parameter, 1)), itr)::Any
│   %4 = Core.fieldtype(%2, 2)::Type{var"#s439"} where var"#s439"<:(Union{Nothing, _B} where _B)
│   %5 = Base.Iterators.iterate(itr)::Any
│        Core.typeassert(%5, %1)::Any
│   %7 = Base.convert(%4, %5)::Any
│   %8 = %new(%2, %3, %7, 0)::Base.Iterators.Stateful{_A,_B} where _B where _A
└──      return %8
) => Base.Iterators.Stateful{_A,_B} where _B where _A

xref #36454. Given the changing landscape, should we reconsider the balance here? cmp seems to be the biggest trouble-spot, so if we do decide to make a change just going back to the raw iteration protocol there might be all that's strictly necessary.

Keno requested a review from StefanKarpinski January 24, 2018 21:08

Keno added iteration Involves iteration or the iteration protocol strings "Strings!" labels Jan 24, 2018

StefanKarpinski reviewed Jan 24, 2018

View reviewed changes

Keno force-pushed the kf/statefulstring branch 3 times, most recently from d1f48aa to ed5c124 Compare January 26, 2018 20:26

vtjnash reviewed Jan 26, 2018

View reviewed changes

vtjnash approved these changes Jan 26, 2018

View reviewed changes

vtjnash reviewed Jan 26, 2018

View reviewed changes

Keno force-pushed the kf/statefulstring branch from ed5c124 to cd78ebd Compare January 27, 2018 20:25

Keno changed the title ~~WIP: Use Stateful in String code~~ Use Stateful in String code Jan 27, 2018

Keno force-pushed the kf/statefulstring branch from cd78ebd to 1cfe250 Compare January 27, 2018 20:59

StefanKarpinski reviewed Jan 27, 2018

View reviewed changes

base/iterators.jl Outdated

fixpoint_iter_type(itrT,

fieldtype(nextvalstate, 1),

fieldtype(nextvalstate, 2)))

end

Copy link

Member

StefanKarpinski Jan 27, 2018

Choose a reason for hiding this comment

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

😮

StefanKarpinski reviewed Jan 27, 2018

View reviewed changes

Keno force-pushed the kf/statefulstring branch from 1cfe250 to 5cc5041 Compare January 27, 2018 21:52

Keno mentioned this pull request Jan 29, 2018

Tune Stateful iterator #25792

Merged

Get rid of uses of raw iter protocol in string code

43ce899

Many cases can now be handled by the new `Stateful` iteration wrapper, other can handled by `pairs(SubString)`. The latter could use some specialized code to avoid decoding UTF-8 twice, but that's not done in this PR.

Keno force-pushed the kf/statefulstring branch from 5cc5041 to 43ce899 Compare January 30, 2018 15:49

Keno merged commit f11d820 into master Jan 30, 2018

ararslan deleted the kf/statefulstring branch January 30, 2018 21:26

Use Stateful in String code #25736

Use Stateful in String code #25736

Conversation

Keno commented Jan 24, 2018

StefanKarpinski left a 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

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

Choose a reason for hiding this comment

Keno commented Jan 26, 2018 • edited Loading

Keno commented Jan 26, 2018

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

vtjnash left a 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

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

nanosoldier commented Jan 27, 2018

Keno commented Jan 27, 2018 • edited Loading

Keno commented Jan 27, 2018

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

Choose a reason for hiding this comment

Keno commented Jan 27, 2018

nanosoldier commented Jan 28, 2018

nanosoldier commented Jan 28, 2018

Keno commented Jan 30, 2018

nanosoldier commented Jan 30, 2018

Keno commented Jan 30, 2018

timholy commented Aug 17, 2020

Keno commented Jan 26, 2018 •

edited

Loading

Keno commented Jan 27, 2018 •

edited

Loading