Skip to content

Use Endo [a] for universe/On/Of implementation#994

Merged
RyanGlScott merged 2 commits intoekmett:masterfrom
phadej:universe-dlist
Dec 11, 2021
Merged

Use Endo [a] for universe/On/Of implementation#994
RyanGlScott merged 2 commits intoekmett:masterfrom
phadej:universe-dlist

Conversation

@phadej
Copy link
Copy Markdown
Collaborator

@phadej phadej commented Dec 9, 2021

I added a benchmark which is maybe exaggerated but highlights the problem:

Benchmark                          plated-master  plated-branch   
universe                           0.823e-4       0.613e-4 -25.52%
universeOf (cloneTraversal plate)  0.824e-4       0.609e-4 -26.08%
universeOf Data.template           2.794e-4       3.370e-4 +20.61%
universeOf Data.tinplate           2.234e-4       3.050e-4 +36.53%
universeOf Data.uniplate           2.789e-4       3.472e-4 +24.47%
universeOf Data.uniplate fibExpr   0.840e-4       0.835e-4  -0.60%
universeOf plate                   0.830e-4       0.611e-4 -26.34%
universeOf plate fibExpr           0.453e-4       0.262e-4 -42.04%
Geometric mean                     1.179e-4       1.075e-4  -8.79%

I don't know why Data based plates are slower though, that is weird.

universeOf :: Getting [a] a a -> a -> [a]
universeOf l = go where
go a = a : foldMapOf l go a
universeOf :: Getting (Endo [a]) a a -> a -> [a]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably the changes to the type signatures in this module warrant a major version bump?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately yes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Can you mention these changes in the CHANGELOG, along with a description of how to migrate code that might be broken by this change? (I'd do so myself, but I'm not sure I understand which situations would break due to these changes.)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If people have Getting [a] values they pass around, they will need to use Getting (Endo [a]), but I doubt anyone does that. Or if they define own combinators (which take in Getting [a] a a), they would need to change signature. So in short: fix type errors GHC tells you to fix.

(It's a unfortunate that Getting r s a has that r argument, leaky implementation detail)

Copy link
Copy Markdown
Collaborator

@RyanGlScott RyanGlScott Dec 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a proposed addition to the CHANGELOG:

5.2 [????.??.??]
----------------
* The type of `universeOf` has changed:

  ```diff
  -universeOf :: Getting       [a]  a a -> a -> [a]
  +universeOf :: Getting (Endo [a]) a a -> a -> [a]
  ```

  In many cases, using `Endo [a]` over `[a]` improves performance. Most call
  sites to `universeOf` will not be affected by this change, although you may
  need to update your code if you define your own combinators in terms of
  `universeOf`.

Does this sound right? If so, I can add this after merging the PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's great! thanks.

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.

2 participants