-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Speed up shuffle #348
Speed up shuffle #348
Conversation
This looks good to me; some benchmarking results would be good though. With regards to bumping containers, I don't think it should be necessary as it looks like the pattern synonym is only used for adding an element, so plain |
I used it for more earlier, but shifted. Yeah, I'll take that out. |
@treeowl, thank you, for all the hard work you've been putting in here and on the other PRs 👍 🍻 Often we're all swamped and so there're delays. (Note to self, perhaps it's time for a Agreed with @HuwCampbell on trying to avoid bumping dependencies when possible. Also, it'd be nice with some benchmarks for future reference, as a comment here, or better in the commit message itself. (Yep, we've had some fairly long commit messages in the past.) Once we resolve those, we could look forward getting this merged /cc @jacobstanley |
I've done everything except benchmarking. I don't really have the time right now to set up Criterion or Gauge benchmarks for this. Messing around in GHCi with moderately long lists seemed a pretty convincing demonstration, and it drops the algorithm from quadratic to linearithmic in both time and allocation. |
86a96c6
to
2b29019
Compare
* Add `shuffleSeq` to shuffle sequences. * Use `shuffleSeq` to implement `shuffle` much more efficiently. The shuffling algorithm we use finds and removes an arbitrary element, which sequences can do in logarithmic time and space rather than linear time and space. It also uses `length`, which is constant time for sequences and linear time for lists.
@moodmosaic any chance you could have another look at this? |
Yep, during the weekend hopefully, as soon as I'm back at my laptop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@treeowl, I've just setup a quick Criterion benchmark. Looking good 👍 💯 See comment below.
Yeah, |
Yep, FTR it was added in 0.5.2 https://hackage.haskell.org/package/containers-0.5.2.0/docs/doc-index-D.html Edit: You are right, it was added in 0.5.8. But since the other one in the Just to avoid the extra maintenance and the CPP is all, but absolutely open for keeping both if there's a valid reason for it. |
OTOH the one in the |
|
Only you can decide what bounds you consider acceptable. I think it would be a shame not to take the improved performance when we can get it. Do you think you could break up the benchmarks a bit? It would be useful to see scaled comparisons (or just text) for 3/30/300 and also 3000 elements. The small end in the graph above is too small to see, and at the large end you don't seem to get quite large enough for the new algorithm to blow the old one out of the water. |
👍 Sounds reasonable, will do and try to upload the full benchmark HTML here. |
The one taking advantage of
|
Add
shuffleSeq
to shuffle sequences.Use
shuffleSeq
to implementshuffle
much more efficiently. The shufflingalgorithm we use finds and removes an arbitrary element, which sequences
can do in logarithmic time and space rather than linear time and
space. It also uses
length
, which is constant time for sequences andlinear time for lists.
Bump lower bound on
containers
. We could avoid this, if necessary,by not using pattern synonyms for sequences.