fraxl
fraxl copied to clipboard
ElvishJerricco
Remove vinyl dependency
This removes the dependency on vinyl which seems to have compilation errors with recent versions of GHC.
Hey thanks! The reason I haven't done this myself is that we really need Data.Union to have the same performance as Data.OpenUnion from freer-effects. Granted, the current implementation of Data.Union is not a regression from the vinyl-plus that fraxl used to use, but the performance of freer-effects's union type was a pretty major innovation in the performance of extensible effects.
Also, I would prefer not to use default-extensions in the cabal file. It obscures information to the reader, and this is primarily a research package after all ;)
Oh, I see you implemented Data.Union yourself here. I had assumed you'd used the union package, which I believe is identical to what you've made. Depending on that package would be better, especially since it has a module namespace conflict with this.
Thanks. I was having trouble with vinyl in my setup for some reason and union also uses it. The strictness annotations are a good point though. Even with that, Data.Union is probably a lot slower than the implementation from freer-effects (which is heavily optimized). Perhaps I will switch to that one.
Yea, I think we need to have a conversation with the author of union to see if they'd be willing to sacrifice the nice, pure implementation of union in favor of the much faster implementation in freer-effects. If they're willing to do that, then we should just depend on union. But otherwise, we'll have to see if we can work with the freer-effects authors on getting their union factored out into a Hackage package somewhere.
Closing this in favor of the CoRec type from vinyl until we find a constant time solution.
This stuff is all a bit above my pay grade but have you looked into fastsum? I think it does some TH to trade compile time for fast runtime. As long as you don't have to rebuild it, it is supposed to be pretty fast.
Thad does seem quite good. I dislike taking the namespace Data.Sum, but that's minor. I'm not convinced that vinyl's CoRec couldn't be made to have faster operations that we need, but it seems pretty nontrivial. So I would probably welcome a PR switching to fastsum.
I think its probably above my abilities to benchmark and add that. I'm sure what you have now is plenty fast for my needs, just wanted to see if you'd seen fastsum and if it would solve any problems.
I have a pretty tenuous understanding of the pieces involved but I was actually able to drop vinyl and replace it with fastsum and ran some benchmarks and saw a small performance increase in some cases.
On master:
stack bench --benchmark-arguments="seql 1000000"
# 18.82s before
stack bench --benchmark-arguments="seqr 1000000"
# 7.00s
stack bench --benchmark-arguments="tree 20"
# 19.17s
With fastsum:
stack bench --benchmark-arguments="seql 1000000"
# 15.79s
stack bench --benchmark-arguments="seqr 1000000"
# 7.08s
stack bench --benchmark-arguments="tree 20"
# 17.98s
This could be coincidental, the benchmarks are only running once, but I'll throw together a PR for you to review.
