function next(eit::EdgeIter, state) now does not modify state

[IssamT]

Fixing #419

It seems that there are some exceptions in julia Base where next modified its arguments, but I don't think it is needed here. Maybe if you have some benchmarks I can run them and see the impact of such modification.

[codecov-io]

Current coverage is 98.14% (diff: 100%)

Merging #423 into master will increase coverage by <.01%

Powered by Codecov. Last update da2a26f...48817c4

[sbromberger]

Can you run a quick benchmark before and after?

g = Graph(10_000,1_000_000)
@time z = collect(edges(g));

ought to test things.

[IssamT]

There are more allocations as expected. Maybe we should keep it as it is without my modifications:

after patch:

0.087874 seconds (1.02 M allocations: 23.315 MB, 19.94% gc time)
0.049320 seconds (1.00 M allocations: 22.888 MB, 21.18% gc time)
0.085568 seconds (1.00 M allocations: 22.888 MB, 32.56% gc time)
0.124874 seconds (1.00 M allocations: 22.888 MB, 67.30% gc time)
0.069871 seconds (1.00 M allocations: 22.888 MB, 27.71% gc time)
0.141565 seconds (1.00 M allocations: 22.888 MB, 67.78% gc time)
0.096521 seconds (1.00 M allocations: 22.888 MB, 28.45% gc time)
0.108220 seconds (1.00 M allocations: 22.888 MB, 66.77% gc time)
0.085522 seconds (1.00 M allocations: 22.888 MB, 26.22% gc time)
0.147368 seconds (1.00 M allocations: 22.888 MB, 71.82% gc time)
0.086941 seconds (1.00 M allocations: 22.888 MB, 24.80% gc time)
0.114960 seconds (1.00 M allocations: 22.888 MB, 70.63% gc time)
0.100419 seconds (1.00 M allocations: 22.888 MB, 28.55% gc time)
0.117108 seconds (1.00 M allocations: 22.888 MB, 67.41% gc time)
0.097648 seconds (1.00 M allocations: 22.888 MB, 27.85% gc time)
0.131176 seconds (1.00 M allocations: 22.888 MB, 69.38% gc time)
0.079808 seconds (1.00 M allocations: 22.888 MB, 27.42% gc time)
0.132156 seconds (1.00 M allocations: 22.888 MB, 66.04% gc time)
0.095013 seconds (1.00 M allocations: 22.888 MB, 26.51% gc time)

before patch

0.079939 seconds (16.54 k allocations: 8.038 MB, 4.63% gc time)
0.059864 seconds (8 allocations: 7.630 MB, 14.42% gc time)
0.071052 seconds (8 allocations: 7.630 MB, 11.16% gc time)
0.057030 seconds (8 allocations: 7.630 MB, 8.51% gc time)
0.045685 seconds (8 allocations: 7.630 MB, 6.36% gc time)
0.056548 seconds (8 allocations: 7.630 MB, 9.12% gc time)
0.042988 seconds (8 allocations: 7.630 MB, 7.70% gc time)
0.057634 seconds (8 allocations: 7.630 MB, 6.93% gc time)
0.055834 seconds (8 allocations: 7.630 MB, 7.63% gc time)
0.049039 seconds (8 allocations: 7.630 MB, 5.99% gc time)
0.048108 seconds (8 allocations: 7.630 MB, 6.92% gc time)
0.050112 seconds (8 allocations: 7.630 MB, 6.60% gc time)
0.052723 seconds (8 allocations: 7.630 MB, 7.71% gc time)
0.057187 seconds (8 allocations: 7.630 MB, 6.31% gc time)
0.066122 seconds (8 allocations: 7.630 MB, 8.54% gc time)
0.060069 seconds (8 allocations: 7.630 MB, 6.25% gc time)
0.056990 seconds (8 allocations: 7.630 MB, 6.36% gc time)
0.041709 seconds (8 allocations: 7.630 MB, 9.59% gc time)
0.051158 seconds (8 allocations: 7.630 MB, 7.83% gc time)
0.063857 seconds (8 allocations: 7.630 MB, 7.48% gc time)
0.059108 seconds (8 allocations: 7.630 MB, 7.01% gc time)
0.054599 seconds (8 allocations: 7.630 MB, 8.30% gc time)
0.048916 seconds (8 allocations: 7.630 MB, 6.82% gc time)
0.057066 seconds (8 allocations: 7.630 MB, 7.52% gc time)
0.051359 seconds (8 allocations: 7.630 MB, 5.99% gc time)
0.054086 seconds (8 allocations: 7.630 MB, 6.77% gc time)
0.055866 seconds (8 allocations: 7.630 MB, 8.53% gc time)
0.070306 seconds (8 allocations: 7.630 MB, 6.70% gc time)
0.051493 seconds (8 allocations: 7.630 MB, 7.85% gc time)
0.061646 seconds (8 allocations: 7.630 MB, 7.22% gc time)

[sbromberger]

Oh, yeah, look at those allocations. Not sure this is a great idea right now, though I understand why we'd want to consider it. Thanks for pulling it together though.

[IssamT]

I don t see surprising info in the amount of allocations. Returning a separate state when iterating 1M times requires 1M allocations. the time doubles in average which is less than what I expected from the allocations. But yes if the package aims to reach the performance edge, this is definitely not a good idea, so feel free to close both the issue and the PR.

[IssamT]

using immutable

0.085918 seconds (15.47 k allocations: 8.019 MB, 4.61% gc time)
0.030574 seconds (7 allocations: 7.630 MB, 21.09% gc time)
0.025852 seconds (7 allocations: 7.630 MB, 14.91% gc time)
0.037600 seconds (7 allocations: 7.630 MB, 7.08% gc time)
0.046564 seconds (7 allocations: 7.630 MB, 6.63% gc time)
0.040199 seconds (7 allocations: 7.630 MB, 5.60% gc time)
0.036929 seconds (7 allocations: 7.630 MB, 4.33% gc time)
0.030815 seconds (7 allocations: 7.630 MB, 5.98% gc time)
0.025692 seconds (7 allocations: 7.630 MB, 8.37% gc time)
0.035490 seconds (7 allocations: 7.630 MB, 4.58% gc time)
0.030208 seconds (7 allocations: 7.630 MB, 8.50% gc time)

EDIT:

The results are even better than before... I m surprised.

[sbromberger]

Now those speeds are compelling :)

[jpfairbanks]

These numbers look close enough to existing performance.

[sbromberger]

Where's the code, though?

Edit: ah, there we go.

[jpfairbanks]

Haha #WheresTheresTheCode

[sbromberger]

Please rewrite this without short-circuiting (per our guidelines):

Style point: prefer the short circuiting conditional over if/else when convenient, and where state is not explicitly being mutated (e.g., condition && error("message") is good; condition && i += 1 is not).

[sbromberger]

@IssamT - thanks for your help with this!

[IssamT]

My thanks to you @sbromberger for helping make the fix better that my first suggestion