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.

Sign up for GitHub

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

Jump to bottom

traverse Array with Maybe more quickly #142

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

traverse Array with Maybe more quickly #142

wants to merge 7 commits into from

Conversation

andrewthad
Copy link
Collaborator

WIP. I still need to handle SmallArray. I also need to make this work with Either, but the backwards-compatibility story there is more annoying because of the historical absence of ExceptT. I also want to do this for State. Probably not Writer though since it has abysmal performance anyway.

@treeowl
Copy link
Collaborator

treeowl commented Apr 22, 2018

I like this. For Either, you can either copy the basics of ExceptT from transformers or (perhaps better) just manually work it into the code for traverseArrayP.

Side note: it's most unfortunate for RULES that we don't have, e.g., Applicative (ExceptT e m) :- Applicative m, and that we have no way to match on the use of a particular instance dictionary either. So all these rules have to be pretty much monomorphic.

@andrewthad
Copy link
Collaborator Author

I've just added Either, State, and Reader and some documentation of what's going on here. To keep things simple, I think I'll just disable the Either optimization if the user builds with transformers older than 0.4. I suspect that creating copies of traverseArrayP for all three of these types that manually inline the monad instance dictionaries (eschewing the use of the monad transformers entirely) doesn't actually cause GHC to generate better code at the use site. I'll add a benchmark soon to confirm this.

Aside from that, are there other issues you see with this? Other types worth supporting?

@treeowl
Copy link
Collaborator

treeowl commented Apr 22, 2018

I don't understand why you think the manual copies (with rewrite rules to match) wouldn't help.

@andrewthad
Copy link
Collaborator Author

I think GHC will produce the same core.

@treeowl
Copy link
Collaborator

treeowl commented Apr 22, 2018 via email

@andrewthad
Copy link
Collaborator Author

Ah, it was consistent behavior with all versions of containers that was your concern. I've changed it as you suggested. Also, I added a benchmark just to see if there was a performance difference between the two implementations. They have identical performance (Wooh GHC!).

@treeowl
Copy link
Collaborator

treeowl commented Apr 22, 2018

All these rewrite rules (including the ones I wrote) feel somewhat unsatisfactory, because they completely fall down for transformers. I'm wondering if there might be a way to catch known transformations of known base monads, using rewrite rules with fall-backs. Roughly speaking, rewrite applications of known transformers recursively until we reach a known base monad (in which case we can rewrite to something particularly efficient) or fail to do so, in which case we ultimately inline to the usual case.

By the way: what sorts of benchmark speed-ups were you able to demonstrate from the Maybe and Either e rules?

@andrewthad
Copy link
Collaborator Author

I've just added a benchmark to compare the two:

benchmarked Array/implementations/traverse/Either/inlined
time                 36.32 μs   (31.90 μs .. 41.36 μs)
                     0.928 R²   (0.884 R² .. 0.998 R²)
mean                 32.20 μs   (31.49 μs .. 33.72 μs)
std dev              3.534 μs   (1.644 μs .. 5.927 μs)
variance introduced by outliers: 65% (severely inflated)

benchmarked Array/implementations/traverse/Either/closure
time                 172.3 μs   (166.3 μs .. 179.7 μs)
                     0.981 R²   (0.965 R² .. 0.994 R²)
mean                 168.8 μs   (166.1 μs .. 172.5 μs)
std dev              10.94 μs   (7.326 μs .. 15.23 μs)
variance introduced by outliers: 42% (moderately inflated)

This is on a pretty noisy box, but there's about a 5x speedup. On PrimArray, it's much more pronounced:

benchmarked PrimArray/traverse/Maybe/Applicative
time                 1.215 ms   (1.191 ms .. 1.237 ms)
                     0.996 R²   (0.991 R² .. 0.999 R²)
mean                 1.181 ms   (1.171 ms .. 1.195 ms)
std dev              41.17 μs   (30.35 μs .. 68.63 μs)
variance introduced by outliers: 17% (moderately inflated)

benchmarked PrimArray/traverse/Maybe/PrimMonad
time                 5.985 μs   (5.817 μs .. 6.189 μs)
                     0.996 R²   (0.993 R² .. 0.999 R²)
mean                 5.858 μs   (5.827 μs .. 5.921 μs)
std dev              135.6 ns   (82.21 ns .. 233.3 ns)

About a 200x speedup on my noisy box. This is because, for PrimArray, the specialized traversals do zero allocations (other than the original allocation of the new array).

I like your idea about recursively rewriting. I'm going to give this a try.

@andrewthad
Copy link
Collaborator Author

andrewthad commented Apr 23, 2018 •
edited
Loading

All these rewrite rules (including the ones I wrote) feel somewhat unsatisfactory, because they completely fall down for transformers. I'm wondering if there might be a way to catch known transformations of known base monads, using rewrite rules with fall-backs.

I can't find a way to make this work. Consider what a rewrite rule for MaybeT might look like:

"traverse/MaybeT" forall (f :: a -> MaybeT m b). traverseArray f = ...

Here's the problem: in the rewrite rule, we have no way to tell GHC that m needs to have a PrimMonad constraint. GHC appears to have a syntax that tricks the user into thinking they can do this, but it doesn't actually work.

@andrewthad
Copy link
Collaborator Author

In fact, in the rewrite rule, GHC doesn't even know that m has an Applicative instance.

@treeowl
Copy link
Collaborator

treeowl commented Apr 23, 2018 via email

@treeowl
Copy link
Collaborator

treeowl commented Apr 23, 2018 via email

@andrewthad
Copy link
Collaborator Author

I'm still not able to follow how this is supposed to look. Going back to MaybeT:

"traverse/MaybeT" forall (f :: a -> MaybeT m b). traverseArray f = ...

I cannot see anything that could go on the RHS (other than traverseArray f) that is both correct and satisfies the type checker.

@andrewthad
Copy link
Collaborator Author

Wait, I think I might be beginning to see a way.

@treeowl
Copy link
Collaborator

treeowl commented Apr 23, 2018 via email

@treeowl
Copy link
Collaborator

treeowl commented Apr 23, 2018

The race is on.

@treeowl
Copy link
Collaborator

treeowl commented Apr 23, 2018 via email

@andrewthad
Copy link
Collaborator Author

Have you pushed these somewhere?

@treeowl
Copy link
Collaborator

treeowl commented Apr 23, 2018 via email

@treeowl
Copy link
Collaborator

treeowl commented Apr 23, 2018 via email

@treeowl
Copy link
Collaborator

treeowl commented Apr 24, 2018

Hrmm... I'm able to write code that at least type checks to handle (some) transformers on top of ST and IO, but I'm actually not sure how to handle transformers on top of Maybe, Either, Identity, etc. Do you think it's doable? If not for the annoying limits on instance discovery, do you think you could write rules? Maybe those could inspire. Anyway, I'll upload what I have within the hour.

@cartazio
Copy link
Contributor

cartazio commented Apr 24, 2018 via email

@treeowl
Copy link
Collaborator

treeowl commented Apr 24, 2018

@cartazio, I've only looked at hermit briefly. It's rather complicated. The basic limitation is basically that the instance resolution derivations drop away after type checking. So for example the simplifier doesn't know how GHC resolved PrimMonad (WriterT w m) and therefore doesn't know Monad m or Monoid w.

@cartazio
Copy link
Contributor

cartazio commented Apr 24, 2018 via email

@treeowl
Copy link
Collaborator

treeowl commented Apr 26, 2018

For stacks based on ST or IO, my version seems much simpler, and involves far fewer enormous piles of code that need to go away. I suggest you just ignore those stacks altogether and try to come up with a solution for stacks based on Maybe, Either, Identity, etc. It should be easy to use both solutions at the same time.

@treeowl
Copy link
Collaborator

treeowl commented Apr 26, 2018

Separately, I'm considering a simpler general version that might work somewhat better than the one I put in before. In particular, we can go back to something somewhat list-like, but using a list that holds multiple elements per cons. I need to do some benchmarking.

@andrewthad
Copy link
Collaborator Author

Undoubtedly, yours is much simpler. In my most recent commit, I was mostly playing around just to see what it takes to make things like Maybe and Either accepted as the base monad. It’s helped me understand the problem better. I have another idea for how to extend your solution to accept different base monads that I am going to try soon.

@cartazio
Copy link
Contributor

Question: why do we need the rewrite rules?

a) is it because we can better specialize the code ?

b) is it because you want to be "Afine/ linear resource safe"?

@treeowl
Copy link
Collaborator

treeowl commented Apr 27, 2018 via email

@treeowl
Copy link
Collaborator

treeowl commented Apr 27, 2018

@cartazio, the fast way to traverse an array is to

  1. Create a mutable array.
  2. For each element of the given array,
    a. Perform an action to get an element and
    b. Write that element to the mutable array.
  3. Freeze the mutable array and return the resulting immutable array.

We'd like traverse to do that whenever it can, but it's not always possible. Even ignoring safety, note that Compose m n is not generally a PrimMonad, or even a Monad, even if m and n both are.

@cartazio
Copy link
Contributor

I'm not terribly familiar with Compose, could you walk me through an example?

@cartazio
Copy link
Contributor

also where does Compose live?

@cartazio
Copy link
Contributor

hrmm

if this is the correct defintion of compose

-- | Right-to-left composition of functors.
-- The composition of applicative functors is always applicative,
-- but the composition of monads is not always a monad.
newtype Compose f g a = Compose { getCompose :: f (g a) }

wouldn't just require PrimMonad just one of these?

@cartazio
Copy link
Contributor

hrmmm, that makes me think that we can't have an instance for compose, because there should only be one primonad in the stack ... i think?

@cartazio
Copy link
Contributor

(something something global approximate linearity of state token)

@cartazio
Copy link
Contributor

i guess i'm perhaps not understnding why Compose the data type/monad enters into this, but i'll have to look at more details

@treeowl
Copy link
Collaborator

treeowl commented Apr 27, 2018 via email

@treeowl
Copy link
Collaborator

treeowl commented Apr 27, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrewthad @treeowl @cartazio