Skip to content
This repository has been archived by the owner on Oct 4, 2020. It is now read-only.

Fold directly #137

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

matthewleon
Copy link
Contributor

@matthewleon matthewleon commented Jan 17, 2018

solves #134

This gives over a 10x speed improvement on larger maps (compare branch https://github.com/matthewleon/purescript-maps/tree/foldl-bench):

before

foldl: midsize map (10000)
mean   = 24.36 ms
stddev = 1.59 ms
min    = 22.57 ms
max    = 31.34 ms
foldl: big map (1000000)
mean   = 3.90 s
stddev = 143.96 ms
min    = 3.75 s
max    = 4.25 s

after

foldl: midsize map (10000)
mean   = 2.32 ms
stddev = 485.38 μs
min    = 2.04 ms
max    = 5.29 ms
foldl: big map (1000000)
mean   = 249.78 ms
stddev = 64.46 ms
min    = 226.67 ms
max    = 433.12 ms

@hdgarrood
Copy link
Contributor

This seems very similar to the implementation used in #132; can this be abstracted into a single function which all of these depend on? I also wouldn't want to have foldl do it the fast way and the other two Foldable member implementations do it the slow way, ideally.

@matthewleon
Copy link
Contributor Author

This seems very similar to the implementation used in #132; can this be abstracted into a single function which all of these depend on?

Will try.

I also wouldn't want to have foldl do it the fast way and the other two Foldable member implementations do it the slow way, ideally.

foldMap is switched to use foldl. I'll try giving foldr a similar rewrite. I'm not sure the benefits would be as drastic, as you won't get TCO.

implement foldl in terms of foldlWithIndex. Effect on benchmarks is
negligible.
Reveals that it is very, very un-stack-safe
@matthewleon
Copy link
Contributor Author

I also wouldn't want to have foldl do it the fast way and the other two Foldable member implementations do it the slow way, ideally.

After attempting to do this, I've restored right folds to using the intermediate lists. Any attempt to do a right fold directly on the Map is not stack-safe.

any attempt at directly folding is not stack safe
@matthewleon
Copy link
Contributor Author

matthewleon commented Jan 17, 2018

This seems very similar to the implementation used in #132; can this be abstracted into a single function which all of these depend on?

I can't see a way to do this. It seems to me that folding and toUnfoldable are in some way "opposites". They bear a superficial similarity, but I can't see have them rely on a common function.

@matthewleon
Copy link
Contributor Author

I've edited down my last two posts here... I believe common functionality between keys, values, and toAscUnfoldable can indeed by refactored. I think that's out of scope for this particular PR though. I will try some strategies on the appropriate PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants