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

fuseSetters function #117

Open
mikesol opened this issue Aug 11, 2020 · 8 comments
Open

fuseSetters function #117

mikesol opened this issue Aug 11, 2020 · 8 comments

Comments

@mikesol
Copy link

mikesol commented Aug 11, 2020

Hello!

We've been using a function called fuseSetters a fair bit in production to speed up code where several setters are called on complex records (in our case, adding custom OpenAPI tags to a large OpenAPI spec).

Here is the function with the output. If folks find it useful, I can make a PR to include it in the library:

module Main where

import Prelude
import Data.Lens (Setter', _1, _2, over)
import Data.Tuple (Tuple(..), fst, snd)
import Effect (Effect)
import Effect.Class.Console (log)

fuseSetters :: forall s a b c. Setter' a b -> Setter' a c -> Setter' s a -> Setter' s (Tuple (b -> b) (c -> c))
fuseSetters a b c l = over c (over a fa <<< over b fb)
  where
  t = l $ Tuple identity identity

  fa = fst t

  fb = snd t

main :: Effect Unit
main = do
  log
    ( show
        $ over
            (fuseSetters (_1 <<< _1) (_2 <<< _2) (_2 <<< _2 <<< _2))
            (const $ Tuple (const 42) (const 53))
            (Tuple 1 (Tuple 2 (Tuple 3 (Tuple (Tuple 0 1) (Tuple 5 8)))))
    )

yields

07:06 meeshkan-abel@Abel:/tmp/lenz$ spago run
[info] Installation complete.
[info] Build succeeded.
(Tuple 1 (Tuple 2 (Tuple 3 (Tuple (Tuple 42 1) (Tuple 5 53)))))

Thanks and let me know!

@thomashoneyman
Copy link
Contributor

Hi @mikesol! This looks interesting! Do you have any metrics on the change, or a snapshot of the difference in generated code? If this were added it would be good to include something like that in the documentation to better understand what this accomplishes.

@mikesol
Copy link
Author

mikesol commented Aug 11, 2020

Below is a benchmark. fuseSetters results in a roughly 1.8x performance increase on the best run. There is no substantial difference in the generated code.

fuseSetters makes sense when working with large and/or deeply-nested data structures in a repeated fashion. The basic idea is that a "trunk" setter focuses up to a certain point in the optic, and then the two "branch" setters focus from that point to two different points.

With fuseSetters

module Main where

import Prelude
import Data.Array (replicate)
import Data.Lens (Setter', _1, _2, over, traversed)
import Data.Tuple (Tuple(..), fst, snd)
import Effect (Effect)
import Effect.Class.Console (log)

fuseSetters :: forall s a b c. Setter' a b -> Setter' a c -> Setter' s a -> Setter' s (Tuple (b -> b) (c -> c))
fuseSetters a b c l = over c (over a fa <<< over b fb)
  where
  t = l $ Tuple identity identity

  fa = fst t

  fb = snd t

setter = (traversed <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2)

main :: Effect Unit
main = do
  void
    ( pure
        $ over
            (fuseSetters (_1 <<< _1) (_2 <<< _2) setter)
            (const $ Tuple (const 42) (const 53))
            ( replicate
                1000000
                (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 2 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 2 (Tuple 3 (Tuple (Tuple 0 1) (Tuple 5 8)))))))))))))))))))))
            )
    )
  log "Done"

Best run

07:45 meeshkan-abel@Abel:/tmp/lenz$ time spago run
[info] Installation complete.
[info] Build succeeded.
Done

real    0m3,191s
user    0m8,221s
sys     0m1,889s

Without fuse setters

module Main where

import Prelude
import Data.Array (replicate)
import Data.Lens (Setter', _1, _2, over, set, traversed)
import Data.Tuple (Tuple(..), fst, snd)
import Effect (Effect)
import Effect.Class.Console (log)

setter = (traversed <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2 <<< _2)

main :: Effect Unit
main = do
  void
    ( pure
        $ ((set (setter <<< _1 <<< _1) 42) <<< (set (setter <<< _2 <<< _2) 53))
            ( replicate
                1000000
                (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 2 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 1 (Tuple 2 (Tuple 3 (Tuple (Tuple 0 1) (Tuple 5 8)))))))))))))))))))))
            )
    )
  log "Done"

Best run

07:44 meeshkan-abel@Abel:/tmp/lenz$ time spago run
[info] Installation complete.
[info] Build succeeded.
Done

real    0m6,320s
user    0m14,908s
sys     0m2,051s

@thomashoneyman
Copy link
Contributor

This seems like a useful utility to add. I don't have as much knowledge around how this would compose -- do you build up nested fuseSetters like fuseSetter a (fuseSetter b c identity) d? -- and I'd appreciate hearing what @LiamGoodacre or @MonoidMusician think before confirming for sure that I'd like to add it to the library. If either of them think it's good as-is then I welcome a PR!

@mikesol
Copy link
Author

mikesol commented Aug 12, 2020

Thanks for the questions and feedback!

Composition looks like this:

module Main where

import Prelude
import Data.Array (replicate)
import Data.Lens (Setter', _1, _2, over, set, traversed)
import Data.Tuple (Tuple(..), fst, snd)
import Effect (Effect)
import Effect.Class.Console (log)

fuseSetters :: forall s a b c. Setter' a b -> Setter' a c -> Setter' s a -> Setter' s (Tuple (b -> b) (c -> c))
fuseSetters a b c l = over c (over a fa <<< over b fb)
  where
  t = l $ Tuple identity identity

  fa = fst t

  fb = snd t

cTuple :: forall a b c. a -> b -> c -> Tuple a b
cTuple a b = const $ Tuple a b

main :: Effect Unit
main = do
  log
    ( show
        $ over (fuseSetters _1 (fuseSetters _1 _2 _2) _2)
            ( cTuple
                ((+) 55)
                (cTuple (flip (-) 101) ((*) 57))
            )
            (Tuple 0 (Tuple 0 (Tuple 1 2)))
    )

yields

06:35 meeshkan-abel@Abel:/tmp/lenz$ spago run
[info] Installation complete.
[info] Build succeeded.
(Tuple 0 (Tuple 55 (Tuple -100 114)))

So the input type is a tree resembling the tree of the fused setters.

@LiamGoodacre
Copy link
Member

fuseSetters :: forall s a b c. Setter' a b -> Setter' a c -> Setter' s a -> Setter' s (Tuple (b -> b) (c -> c))
fuseSetters a b c l = over c (over a fa <<< over b fb) ...

The over c part seems a little redundant?

fuseSettersV2 :: forall a b c. Setter' a b -> Setter' a c -> Setter' a (Tuple (b -> b) (c -> c))
fuseSettersV2 ba ca l = (over ba fa <<< over ca fb) ...

where

fuseSetters b2a c2a a2s
-- is
a2s <<< fuseSettersV2 b2a c2a

@mikesol
Copy link
Author

mikesol commented Aug 12, 2020

Nice find! It makes the syntax cleaner too. I've updated the PR.

module Main where

import Prelude
import Data.Array (replicate)
import Data.Lens (Setter', Setter, _1, _2, over, set, traversed)
import Data.Tuple (Tuple(..), fst, snd)
import Effect (Effect)
import Effect.Class.Console (log)

fuseSetters :: forall a b c. Setter' a b -> Setter' a c -> Setter' a (Tuple (b -> b) (c -> c))
fuseSetters ba ca l = (over ba fa <<< over ca fb)
  where
  t = l (Tuple identity identity)

  fa = fst t

  fb = snd t

cTuple :: forall a b c. a -> b -> c -> Tuple a b
cTuple a b _ = Tuple a b

main :: Effect Unit
main = do
  log
    ( show
        $ over
            (_2 <<< (fuseSetters _1 (_2 <<< (fuseSetters _1 _2))))
            ( cTuple
                ((+) 55)
                (cTuple (flip (-) 101) ((*) 57))
            )
            (Tuple 0 (Tuple 0 (Tuple 1 2)))
    )

@MonoidMusician
Copy link
Contributor

I'm not sure what value this provides. Isn't the alternative to setter <<< fuseSetters (_1 <<< _1) (_2 <<< _2) not (set (setter <<< _1 <<< _1) 42) <<< (set (setter <<< _2 <<< _2) 53) but over setter (set (_1 <<< _1) 42 <<< set (_2 <<< _2) 53)? (Sorry I didn't run that through a typechecker.) If the same idea and performance can be accomplished within the existing idiom, it seems cleaner to me to do so, especially to avoid dealing with const/cTuple. Unless there's some value added by using Setters here that I'm missing …

After looking over it again, is it just the ability to separate the data from the setter while maintaining performance? I wonder if there's a way to achieve that without using Setter' a (Tuple (b -> b) (c -> c)) and so many consts, because I would think that having to rewrite the data to use consts would make it less convenient to use, but idk … Would Setter' a (Tuple b c) work as the return type?

Actually, after some more research, I think that Setter' a b -> Setter' a c -> Setter' a (Tuple b c) (and probably fuseSetters already) runs into the issue that it's not lawful when the lenses/setters overlap, c.f. https://stackoverflow.com/a/23322201.

@mikesol
Copy link
Author

mikesol commented Aug 17, 2020

This is a bit over my head, but what I would say is that if the idiom you've implemented works, it's much better to use that than fuseSetters. At the end of the day, we use fuseSetters to speed up performance, so anything that achieves a similar effect in a more elegant way is much better. The only issue is that we have it in production code, so it may be a while before we can test it out thoroughly.

Another reason not to include it would be that it's too use-case specific. Most folks won't need speed optimization on setters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants