Implement foldMap1 for arrays in JavaScript to fuse mapping and folding

[kl0tl]

The default implementations of foldMap1 have to map over the array before folding it, but we can avoid to traverse the array twice with a foreign implementation.

[hdgarrood]

I think we should try to avoid introducing more JS code here, for two reasons:

• the more we drop down to JS, the less incentive we have to improve the tools for writing efficient PureScript code for this sort of thing (eg ST)
• upgrading becomes more of a pain for alternate backend maintainers the more JS there is

[JordanMartinez]

the more we drop down to JS, the less incentive we have to improve the tools for writing efficient PureScript code for this sort of thing (eg ST)

AFAICT, there is incentive for improving ST. In my mind, this is only a temporary workaround because we don't have STFn.

upgrading becomes more of a pain for alternate backend maintainers the more JS there is

Is one function going to make that much of a difference?

[hdgarrood]

Right, that’s exactly my point; if we just do this in JS then there won’t be much of an incentive to work on STFn (or some other kind of mechanism that alleviates the same issue). It’s not obvious to me that the current implementation is a problem in practice.

Yes, even one function is a pain, because it means backend maintainers have to write and test new code in the FFI modules in the backend language that they have to maintain alongside each library. Of course it won’t just be one function if we get into the habit of dropping down to JS whenever there’s a potential performance boost to be gained, too.

[natefaubion]

I don't think it would be difficult to write tail-recursively with unsafeIndex, and I feel like you'd get essentially the same thing?

[JordanMartinez]

if we just do this in JS then there won’t be much of an incentive to work on STFn (or some other kind of mechanism that alleviates the same issue).

AFAICT, there is incentive to work on STFn. purescript/purescript-st#31 sums it up. Adding one function doesn't lessen this incentive, only increases it. If anything, those maintaining non-JS backends would probably voice their support that we work on STFn so that they don't have to keep doing this. In my mind, the fact of "adding more FFI increases backend maintenance" can be interpreted either as something that will motivate or demotivate us to work on STFn.

[hdgarrood]

I think it’s our job to try to reduce downstream breakage as much as is reasonably possible in the first place, rather than just doing it anyway and waiting for complaints in order to decide whether or not to keep doing it. I don’t think this performance improvement is enough to justify the breakage. Although I think @natefaubion is right that a tail-recursive implementation based on unsafeIndex ought to work, and that gets us the best of both worlds.

[JordanMartinez]

I think it’s our job to try to reduce downstream breakage as much as is reasonably possible in the first place, rather than just doing it anyway and waiting for complaints in order to decide whether or not to keep doing it. I don’t think this performance improvement is enough to justify the breakage.

I agree with that. Also, I don't think I was considering your other point with as much weight as I should have: "...it won’t just be one function if we get into the habit of dropping down to JS whenever there’s a potential performance boost to be gained, too."

Since we'll be working on ES builds in v0.15.0, could STFn be another core feature that is planned to be implemented by then, too (assuming we don't do it sooner)?

[JordanMartinez]

a tail-recursive implementation based on unsafeIndex ought to work

I tried implementing this locally. Unfortunately, it doesn't work due to cyclical module dependencies:

[1/1 CycleInModules] src/Data/Array.purs:30:1

       v
   30  module Data.Array
   31    ( fromFoldable
   32    , toUnfoldable
       ...
  146  import Data.Unfoldable (class Unfoldable, unfoldr)
  147  import Partial.Unsafe (unsafePartial)
  148  import Prim.TypeError (class Warn, Text)
                                              ^
  
  There is a cycle in module dependencies in these modules:
  
    Data.Array
    Data.Array.NonEmpty.Internal

           Src   Lib   All
Warnings   0     0     0  
Errors     1     0     1  

unsafeIndex is in Data.Array, which imports Data.Array.NonEmpty for usage in group* functions.

[JordanMartinez]

We could break the cyclical dependency by moving unsafeIndex into a Data.Array.Unsafe module, but not sure it's worth it.

[natefaubion]

Should Array.NonEmpty not have it's own unsafeIndex?

[natefaubion]

I don't think we want to move Array.unsafeIndex since there are optimization rules around it in the compiler. We can add Array.NonEmpty.unsafeIndex, but this would be another FFI addition. It's arguable that this function should exist for feature parity with Array, however. If FFI additions are a hard pass, then I think the status quo is the only thing that works for now.

[JordanMartinez]

I don't think we want to move Array.unsafeIndex since there are optimization rules around it in the compiler. We can add Array.NonEmpty.unsafeIndex, but this would be another FFI addition.

Would we add that in v0.15.0 when the next round of breaking changes occur?

[natefaubion]

Would we add what, specifically?

[hdgarrood]

I don't think FFI additions should be a hard pass, they just need to be able to justify themselves. I think adding an FFI unsafeIndex to Data.Array.NonEmpty is justifiable for feature parity and to avoid breaking the inlining rules in the compiler.

[JordanMartinez]

Add unsafeIndex to Data.Array.NonEmpty.Internal by using the migration approach described above. Data.Array.NonEmpty does have unsafeIndex by adapting Data.Array.unsafeIndex, but we can't use it in the Internal module due to the above cyclical dependency issue.

[natefaubion]

I think we could add foreign import unsafeIndex :: forall a. NonEmptyArray a -> Int -> a to NonEmpty.Internal, and NonEmpty could use that instead of Array. There's no reason they must share the same implementation, but it's obviously ideal since NonEmpty is a newtype.

[JordanMartinez]

There's no reason they must share the same implementation, but it's obviously ideal since NonEmpty is a newtype.

Would this break the inliner rules in the compiler?

[natefaubion]

Would this break the inliner rules in the compiler?

Unless the compiler already implements inliner rules for NonEmpty specifically, then they are already broken for NonEmpty, or rather never inlined.

[hdgarrood]

Oh, I forgot that NonEmpty already provides an unsafeIndex. In that case I think my vote is to leave things as they are for now. In the longer term I think we should try to stop this kind of awkward situation from happening by a) having some kind of runtime with a defined interface so that alternate backends know what they have to support (and full-featured enough that it can remove most/all of the FFI in core) and b) finding a way of not having the compiler need to know about any downstream code. But these are both fairly big projects.

[JordanMartinez]

Based on the feedback here, I say we close this PR and leave things as is.

[kl0tl]

Let’s close this.