Skip to content
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

Optimize folds #151

Closed
wants to merge 4 commits into from
Closed

Optimize folds #151

wants to merge 4 commits into from

Conversation

sergv
Copy link
Contributor

@sergv sergv commented Feb 13, 2018

The idea is to manually remove arguments that do not change between invocations from worker functions in folds. Ideally, ghc should do this and this PR will not be needed, but 8.2.2 currently doesn't seem to do that. I'll try to provide more research there, but in the meantime it seems worthwhile to optimize bytestring package manually. Thanks to it's backwards compatibility with previous versions of GHC, benefits of optimization will become available there as well.

The speedup is not eye-widening for foldls like foldl', but is pretty good for mapAccumR (around 75%, which could make a good candidate for Haskell performane attack if it wasn't cancelled already). Please find benchmarks at https://gist.github.com/sergv/b4642943db3f59293531c5291e3e3af7.

@sjakobi
Copy link
Member

sjakobi commented Dec 18, 2019

Sounds great!

Are all of the changed functions well-tested?

Regarding the benchmark results, could you post the relative speedups for each function? (I'm not sure about the best tool for that, but you could try https://github.com/composewell/bench-show.)


EDIT: For reference, another tool for comparing benchmarks: https://hackage.haskell.org/package/criterion-compare

@sjakobi
Copy link
Member

sjakobi commented Dec 18, 2019

Ideally, ghc should do this and this PR will not be needed, but 8.2.2 currently doesn't seem to do that.

It would be interesting to know how this PR affects performance with different GHC versions. Could you maybe post results both for 8.2 and 8.10?

@sergv
Copy link
Contributor Author

sergv commented Dec 18, 2019

I've conducted some preliminary benchmarking at https://gist.github.com/sergv/b4642943db3f59293531c5291e3e3af7 (also mentioned in the first post). Not sure how to quickly get relative speedups out of those, but for strings of length 65536 the difference is 8.628 ms against 1.296 ms - a staggering 6.65 times faster.

However I notice that this PR is almost two years old. I do not feel it is adequate to not get any response at all, not even a "thanks", for two years for a core library used by quite a few projects. This is discouraging and it made me rather reluctant to do any more work on this PR. Today's email reinforces my impression that bytestring is not likely to become maintained, in the sense like other popular packages are, any time soon. Therefore I'm sorry, but relative speedups or more benchmarks are not coming from me on this one - it's beyond too late for that.

@sjakobi
Copy link
Member

sjakobi commented Dec 18, 2019

@sergv I can understand your response – I'd probably feel the same in your shoes.

I hope that we can change the maintenance situation so contributions like yours can can get a reaction and be accepted in a more timely fashion.

I'll respond more on the mailing list.

@sjakobi
Copy link
Member

sjakobi commented Dec 18, 2019

Also, thanks @sergv! :)

I'll try to help get this merged, so bytestring users can finally benefit from these performance improvements.

@sergv
Copy link
Contributor Author

sergv commented Dec 19, 2019

@sjakobi Many thanks for pushing through on this.

@sergv
Copy link
Contributor Author

sergv commented Mar 15, 2020

I'm closing this as I'm not going to push further on this. Feel free to take the commits into a new PR.

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

Successfully merging this pull request may close these issues.

3 participants