Change IteratorSize on EnumerableMap to just pass through from underl…

[quinnj]

…ying iterator

In a benchmark I'm running this made about a 10% impact on performance; the problem is that the current code is non-compile-time inferrable, so it involved doing runtime dispatch on a trait meant to be compile-time friendly.

[codecov-io]

Codecov Report

Merging #28 into master will decrease coverage by 0.37%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   79.74%   79.36%   -0.38%     
==========================================
  Files          19       19              
  Lines         474      475       +1     
==========================================
- Hits          378      377       -1     
- Misses         96       98       +2

Impacted Files	Coverage Δ
src/source_iterable.jl	77.77% <0%> (ø)	⬆️
src/enumerable/enumerable.jl	0% <0%> (ø)	⬆️
src/enumerable/enumerable_map.jl	83.33% <0%> (-8.34%)	⬇️
src/enumerable/enumerable_orderby.jl	33.84% <0%> (ø)	⬆️
src/enumerable/enumerable_take.jl	72.22% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35eb87c...41c7551. Read the comment docs.

[davidanthoff]

On one hand this looks fine, but on the other hand I'm worried that I probably introduced the more elaborate thing for some reason. Of course, now I can't remember/think of one...

We could probably get the same semantics as the old version but without the perf problems by using a generated function?

[quinnj]

Most of the other definitions of IteratorSize in QueryOperators are like:

(Base.IteratorSize(S) isa Base.HasLength || Base.IteratorSize(S) isa Base.HasShape) ? Base.HasLength() : Base.SizeUnknown()

so I wonder if you were going for a similar type thing here? Using the above definition would be much better, since it's inferrable.

[quinnj]

Oh, another quick question while we're here: any reason we don't declare IteratorInterfaceExtensions.isiterable(::Enumerable*) in this package? Would you be open to a PR adding those definitions? It cleans up codegen and avoids the applicable call (even though it's relatively negligible compared to everything else).

[davidanthoff]

Yes, I think the two IteratorSize things are equivalent, right? I'm pretty sure there is no deeper reason that I've used a different formulation for the map case, and if the other formulation works better with the compiler, then lets go with that!

And yes, adding a isiterable for Enumerable also seems like a good idea.

[nalimilan]

Have you also tried Base.IteratorSize(S) isa Union{Base.HasLength, Base.HasShape}? Might save one call.

[davidanthoff]

@quinnj let me know if you want to try @nalimilan's suggestion or just merge this right away!

[quinnj]

Updated.