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

WIP: split next into nextval & nextstate #9182

Closed
wants to merge 1 commit into from
Closed

Conversation

nolta
Copy link
Member

@nolta nolta commented Nov 27, 2014

This is a more speculative and breaking fix for #9178. It splits next into nextval and nextstate, as previously discussed in #6125.

For example,

for x in xs
    ...
end

would transform into

iter = start(xs)
while !done(xs,iter)
    x = nextval(xs,iter)
    ...
    iter = nextstate(xs,iter)
end

@nolta nolta added speculative Whether the change will be implemented is speculative breaking This change will break code labels Nov 27, 2014
@eschnett
Copy link
Contributor

Why not call it currval? Presumably it can be called multiple times,
without advancing the iterator.

@johnmyleswhite
Copy link
Member

This is a cool approach. FWIW, I think using a Nullable would be a little simpler.

@nolta
Copy link
Member Author

nolta commented Nov 30, 2014

@johnmyleswhite Do you mean something like this:

function nextmaybe(a::AbstractArray, i)
    i > length(a) ? Nullable() : Nullable(a[i]), i+1
end

iter = start(xs)
while true
    maybe,iter = nextmaybe(xs,iter)
    maybe.isnull && break
    x = maybe.value
    ...
end

i.e., #8149 (comment)?

This PR basically implements C++-style iterators, while the above is more Python-like (Nullable taking the place of the StopIteration exception).

I guess i feel this PR is a little simpler:

@eschnett Sure, nextval isn't necessarily the best name. I'm partial to iterval myself.

@timholy
Copy link
Member

timholy commented Dec 14, 2014

There's some evidence in #9080 that---at least on some architectures---a version of this PR might enable higher performance. It would appear to mandate putting nextstate at the end of the loop, which is currently marked "TBD" in the code.

For example,

    for x in xs
        ...

now transforms into

    iter = start(xs)
    while !done(xs,iter)
        x = nextval(xs,iter)
        ...
        iter = nextstate(xs,iter)
@nolta
Copy link
Member Author

nolta commented Dec 14, 2014

Ok, i've moved nextstate to the end.

@timholy
Copy link
Member

timholy commented Dec 14, 2014

Cool! I'll have to test this on #9080, but I gotta run for now.

@timholy
Copy link
Member

timholy commented Dec 24, 2014

Sorry for the long delay. This indeed helps #9080 a bit:

# Master
julia> @time sumcart_manual(A)
elapsed time: 0.140875669 seconds (96 bytes allocated)
5.000122419731768e7

julia> @time sumcart_iter(A)
elapsed time: 0.215551555 seconds (96 bytes allocated)
5.000122419731768e7


# This branch
julia> @time sumcart_manual(A)
elapsed time: 0.142305083 seconds (96 bytes allocated)
5.0000425472199745e7

julia> @time sumcart_iter(A)
elapsed time: 0.197485609 seconds (96 bytes allocated)
5.0000425472199745e7

It doesn't eliminate the gap, but it is a 10% speed improvement, or equivalently erases 25% of the gap.

I was also hoping it would help the performance of enumerate, but unfortunately that doesn't seem to be the case. Looking at the LLVM and native code, it doesn't fully elide the tuples (even though it doesn't allocate any memory in julia).

On balance I like this change; I suppose the question is whether it is worth the breakage.

@nolta nolta closed this Aug 1, 2015
@StefanKarpinski
Copy link
Member

For what it's worth, I'm still interested in doing this. I guess the open issues would be how to do it without breaking everything. Might be a good change for the Arraymageddon.

@Keno
Copy link
Member

Keno commented Aug 1, 2015

Yes, something is not quite right about iteration yet. I think the most common awkward situation is that you don't know whether you're done until you try to fetch the next value, which doesn't fit cleanly into our current iteration model.

@StefanKarpinski
Copy link
Member

Cross reference: #8149

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code speculative Whether the change will be implemented is speculative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants