Skip to content

Export specialized folds #40

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

Closed
wants to merge 1 commit into from
Closed

Export specialized folds #40

wants to merge 1 commit into from

Conversation

kl0tl
Copy link
Member

@kl0tl kl0tl commented Oct 24, 2020

It seems curious to only export a specialized foldl1, especially now that Foldable1 has fold1 and foldr1 members.

@thomashoneyman
Copy link
Member

It seems curious to export these at all, if they're already members of Foldable1, which anything depending on this library would already bring in. Is there a reason to export these functions instead of keeping them internal and available via the Foldable1 class?

As far as the implementations 👍

@JordanMartinez
Copy link
Contributor

Did you miss Harry's comment in #39

@hdgarrood
Copy link
Contributor

That comment was in the context of whether to export the monomorphic foldl1 or to re-export the general one; the question here is whether we should export monomorphic folds at all. I don’t have a strong opinion either way here but if we decide we don’t want to export monomorphic folds, can we deprecate the existing foldl1 first rather than removing it?

@JordanMartinez
Copy link
Contributor

Gotcha.
I'm for deprecating it. Correct me if I'm wrong, but I assume that monomorphic functions are more performant because a type class dictionary isn't being passed around. So, if this monomorphic function is reusing the type class member as its definition, why export it at all?

@hdgarrood
Copy link
Contributor

It’s true that there’s a performance cost to type class constraints, but in many cases it’s negligible. If you’re using a specialized monomorphic version, the dictionary argument will have already been applied by the time you use it, so that cost has been paid. Also, there’s more to it than just performance: monomorphic versions of functions can mean that explicit type annotations aren’t necessary where they otherwise would have been, and they can also make code easier to read because you can immediately see what type you’re working with rather than having to infer it from context.

@kl0tl
Copy link
Member Author

kl0tl commented Oct 25, 2020

I had the impression that re-exporting specialized versions for better performance (null and length from Data.Array, or null from Data.List for instance) or type inference (purescript/purescript-arrays#173) was something we wanted to do. I don‘t mind adding a Warn constraint to foldl1 to deprecate it if I misunderstood.

@hdgarrood
Copy link
Contributor

Oh, I think I see what the confusion might be. When you’re comparing performance of monomorphic versus polymorphic functions, you can’t jump to conclusions, you really need to look at how they are implemented. Sometimes, in the case of e.g. length :: forall f a. Foldable f => f a -> Int, the polymorphic version is necessarily slower than a monomorphic version, because it has access to less information. In the case of length, it’s not part of the Foldable class, which means that implementations can only make use of what the class provides. In practice that means that length must essentially convert the argument to a list and then measure its length, which is O(n). This is unfortunate in the case of some Foldables such as Array, where there are more efficient ways to compute the length.

In this case, the functions we are dealing with are part of the class, which means that their implementations need to be monomorphic anyway, so the performance difference is just one extra dictionary argument. While this extra cost isn’t nothing, it is pretty much negligible in comparison to the length issue.

@kl0tl kl0tl closed this Nov 19, 2020
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.

4 participants