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

Initial work implenting with day #10

Closed
wants to merge 4 commits into from
Closed

Initial work implenting with day #10

wants to merge 4 commits into from

Conversation

ethul
Copy link
Owner

@ethul ethul commented Dec 3, 2016

@paf31 I have an initial attempt at implementing in terms of Day. It's not quite done yet and I have to get the tests working, but it is almost there. I'd definitely be interested in any thoughts you might have. Thanks!

foldFreeAp

foldfreeap-71acf67-9ccd7db-small

retractFreeAp

retractfreeap-71acf67-9ccd7db-small

@ethul
Copy link
Owner Author

ethul commented Dec 4, 2016

Note that the above benchmarks are not reflective of the latest changes in this branch.

The changes show below had an impact on the performance characteristics. Specifically, the update to the Apply instance. Perhaps there is a better way to write this. Or even remove the Functor constraint.

5f9532d...9cc4b60

-instance applyFreeAp :: Apply f => Apply (FreeAp f) where
+instance applyFreeAp :: Functor f => Apply (FreeAp f) where
   apply (Pure k) f = k <$> f
-  apply (Ap d) (Pure k) = Ap ((#) k <$> d)
-  apply (Ap d) (Ap e) = Ap (d <*> e)
+  apply (Ap d) e = runDay (\i f g -> Ap (day (\x (Tuple y a) -> i x y a) f (pure Tuple <*> g <*> e))) d
 
-instance applicativeFreeAp :: Applicative f => Applicative (FreeAp f) where
+instance applicativeFreeAp :: Functor f => Applicative (FreeAp f) where
   pure = Pure

foldFreeAp (small)

foldfreeap-71acf67-9ccd7db-dat-small

retractFreeAp (small)

retractfreeap-71acf67-9ccd7db-day-small

@ethul
Copy link
Owner Author

ethul commented Dec 4, 2016

Benchmark results (small and large) removing the usage of Tuple and the Functor constraint on Apply.

9cc4b60...d2f589d

-instance applyFreeAp :: Functor f => Apply (FreeAp f) where
-  apply (Pure k) f = k <$> f
-  apply (Ap d) e = runDay (\i f g -> Ap (day (\x (Tuple y a) -> i x y a) f (pure Tuple <*> g <*> e))) d
+instance applyFreeAp :: Apply (FreeAp f) where
+  apply (Pure k) (Pure a) = Pure (k a)
+  apply (Pure k) (Ap d) = Ap (k <$> d)
+  apply (Ap d) e = runDay (\i f g -> Ap (day (#) f (pure (\y a -> (\x -> i x y a)) <*> g <*> e))) d
 
-instance applicativeFreeAp :: Functor f => Applicative (FreeAp f) where
+instance applicativeFreeAp :: Applicative (FreeAp f) where
   pure = Pure

foldFreeAp (small)

foldfreeap-71acf67-9ccd7db-day-small

retractFreeAp (small)

retractfreeap-71acf67-9ccd7db-day-small

foldFreeAp (large)

foldfreeap-71acf67-9ccd7db-day-large

retractFreeAp (large)

retractfreeap-71acf67-9ccd7db-day-large

@paf31
Copy link

paf31 commented Dec 4, 2016

Hmm, it's a little bit of a shame about foldFreeAp and retractFreeAp, but the implementation is quite nice 😄

I wonder if you could move the Ap outside the call to runDay. Would that work? Not sure if it would help or not though.

Do you have any other thoughts on why those might be slower?

@ethul
Copy link
Owner Author

ethul commented Dec 4, 2016

Thanks! I am liking the representation using Day too.

Not sure yet why we have the performance difference in the (small) case. Good suggestion about moving Ap outside of the runDay call. Definitely worth a try! However, in the large cases, the performance between #9 and #10 isn't really noticeable.

@ethul
Copy link
Owner Author

ethul commented Dec 4, 2016

Experiment moving Ap out of runDay.

foldFreeAp (small)

foldfreeap-71acf67-9ccd7db-day-small

@safareli
Copy link
Contributor

Regarding stack issue, functor instance of Day is not safe as it accumulates fs
https://github.com/paf31/purescript-day/blob/v9.0.0/src/Data/Functor/Day.purs#L68-L69

Therefore map of the FreeAp is not safe. apply part here apply (Pure k) (Ap d) = Ap (k <$> d), is also unsafe as map on Day is not safe. I think third case in apply of freeap is also unsafe as it cals apply as well as map on freeap nodes.

The implementation is nicer then one in #9 but both are unsafe in terms of stack size.

I would like to try to open PR with something like this #9 (comment) but written in PS so let's see

@ethul
Copy link
Owner Author

ethul commented Dec 19, 2017

Sounds good. I would be interested in a PR for this. Thanks!

@ethul ethul mentioned this pull request Jan 13, 2018
@ethul
Copy link
Owner Author

ethul commented Jan 27, 2018

Superseded by #13

@ethul ethul closed this Jan 27, 2018
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.

3 participants