Optimize foldMap implementations with combineAll

[carymrobbins]

No description provided.

[codecov-io]

Codecov Report

Merging #2024 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2024      +/-   ##
==========================================
+ Coverage   95.08%   95.09%   +<.01%     
==========================================
  Files         301      301              
  Lines        4946     4953       +7     
  Branches      125      117       -8     
==========================================
+ Hits         4703     4710       +7     
  Misses        243      243

Impacted Files	Coverage Δ
...eycats-core/src/main/scala/alleycats/std/set.scala	100% <ø> (ø)	⬆️
...s-core/src/main/scala/alleycats/std/iterable.scala	100% <ø> (ø)	⬆️
core/src/main/scala/cats/instances/list.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/data/NonEmptyList.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/instances/queue.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/instances/stream.scala	96.49% <100%> (+0.06%)	⬆️
core/src/main/scala/cats/instances/sortedSet.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/instances/vector.scala	71.42% <100%> (+0.69%)	⬆️

---

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 aeff8a6...eb15caa. Read the comment docs.

[johnynek]

This looks great! One question about the Set foldMap which I think should also use .iterator.

[johnynek]

fa.iterator.map(f) will be faster (not allocate a set) also maybe more correct. fa.map(f) if f = { _ => 1 } will create a set of 1 thing. If you do foldMap what do we mean? I actually don't know if this should return the count of items (which I think it should, which means we should do fa.iterator.map), or if it should return 0 if empty and 1 otherwise.

I guess since this is lawless especially we need a comment as to the behavior.

[carymrobbins]

That's a good point, I originally avoided using .iterator to maintain the same semantics, but I forgot to confirm that was the case.

@ alleycats.std.SetInstances.setTraverse.foldMap(Set(1,2,3))(_ => "a")
res13: String = "aaa"

Seems not using .iterator actually would be a breaking change, so I'll switch to use it. Nice catch!

[kailuowang]

Do you guys mind to provide some enlightenment here? I must've missed something, I have trouble understanding how this optimize over the default implementation.
combineAll(as) is doing as.foldLeft(empty)(combine), replace as with as.iterator.map(f), then you get as.iterator.map(f).foldLeft(B.empty)(B.combine),
Default implementation of as.foldMap is doing
as.foldLeft(fa, B.empty)((b, a) => B.combine(b, f(a)))
Why doing map first then foldLeft with combine is better than foldLeft with combineing with f(a) ongoing?

[tpolecat]

combineAll is optimized for some types, which fixes some N^2 cases (like String via StringBuilder or always concatenating lists right to left).

[kailuowang]

thanks @tpolecat .

[johnynek]

👍