Not enough strictness in fold #6

Closed
snoyberg opened this Issue May 30, 2012 · 2 comments

2 participants

@snoyberg

This is an issue that came up in conduit, and would likely occur in pipes-core as well. It's also the same issue as foldl vs foldl' from the base libraries. The fix is simple: just apply seq to the accumulator. For example, here's the conduit code.

I don't think there's really a use case for lazy fold, we certainly haven't had one in conduit.

@snoyberg snoyberg added a commit to snoyberg/pipes-core that referenced this issue May 30, 2012
@snoyberg snoyberg Strictness in fold accumulator (pcapriotti/pipes-core#6) 2dda0c5
@snoyberg

I added pipes-core to the conduit fold benchmark and compared the current version and my modified version with seq. The results are:

benchmarking bigsum-pipes-core
mean: 309.0762 us, lb 306.0745 us, ub 312.0476 us, ci 0.950
std dev: 15.30500 us, lb 13.41773 us, ub 17.62190 us, ci 0.950
found 2 outliers among 100 samples (2.0%)
  2 (2.0%) low mild
variance introduced by outliers: 47.470%
variance is moderately inflated by outliers

benchmarking bigsum-pipes-core-seq
mean: 217.3475 us, lb 216.4075 us, ub 218.2208 us, ci 0.950
std dev: 4.641443 us, lb 4.029243 us, ub 5.461898 us, ci 0.950
found 1 outliers among 100 samples (1.0%)
variance introduced by outliers: 14.229%
variance is moderately inflated by outliers

The difference is greater for larger inputs.

@pcapriotti
Owner

Thanks! I agree that there's little reason to have a lazy fold.

@pcapriotti pcapriotti added a commit that referenced this issue Jun 3, 2012
@pcapriotti Make fold strict (#6).
There doesn't seem to be any use case for a lazy fold, so this commit
makes the `fold` combinator strict, which should improve performance in
most cases.

Patch by Michael Snoyman.
631de8f
@pcapriotti pcapriotti closed this Jun 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment