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

Stack safe version #13

Merged
merged 4 commits into from
Jan 27, 2018
Merged

Stack safe version #13

merged 4 commits into from
Jan 27, 2018

Conversation

safareli
Copy link
Contributor

@safareli safareli commented Jan 2, 2018

fixes #12

It can be optimize even further but generally i'm satisfied with performance we have now (typelevel/cats#1921)

@safareli
Copy link
Contributor Author

@natefaubion @paf31 @ethul @garyb I have added tests and i'm confident that it's correct. So this is ready for review. 🎉

@safareli safareli changed the title WIP stack safe version Stack safe version Jan 13, 2018
@ethul
Copy link
Owner

ethul commented Jan 13, 2018

Thanks so much for your work on this! Your proposal looks great!

I am still working through the changes, but I am wondering if you happen to have any performance numbers that we can use to compare this to the current implementation and the implementations in #9 and #10.

Would you be willing to include graphs, as a comment in this PR, like the ones in the other PRs? Thanks again!

@safareli
Copy link
Contributor Author

I remember you had a lib ps-freeap-benchmarks or something but i can't find it any more, do you have a link to it?

@ethul
Copy link
Owner

ethul commented Jan 13, 2018

Yes! Good call. Are you thinking of this?
https://github.com/ethul/purescript-benchmarks

@safareli
Copy link
Contributor Author

yes it's that, remembered different name :d

safareli added a commit to safareli/purescript-benchmarks that referenced this pull request Jan 13, 2018
safareli added a commit to safareli/purescript-benchmarks that referenced this pull request Jan 13, 2018
@safareli
Copy link
Contributor Author

I have added this version of FreeAp to the repo and did benchmarks https://github.com/ethul/purescript-benchmarks/pull/1/files

Now I'm testing only "New" "Day" and "Safe" versions for big inputs (until first two crash :p)

@ethul
Copy link
Owner

ethul commented Jan 13, 2018

Great! Thanks for doing the benchmarks.

@safareli
Copy link
Contributor Author

I have added some larger benchmarks ethul/purescript-benchmarks@04a8762

To sum up it has almost the same performance as thunk based version here #9

@ethul
Copy link
Owner

ethul commented Jan 27, 2018

This all looks great! Thanks again for your work on this PR.

@ethul ethul merged commit 9025683 into ethul:master Jan 27, 2018
ethul pushed a commit to ethul/purescript-benchmarks that referenced this pull request Jan 27, 2018
* add stacksafe version of FreeAp

from ethul/purescript-freeap#13

* add more large tests
@safareli safareli deleted the stack branch January 28, 2018 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack safety
2 participants