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

Use indexed-traversable #951

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Use indexed-traversable #951

merged 1 commit into from
Dec 16, 2020

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Dec 12, 2020

TODO:

  • package name
  • Bump free version (once it has instances)

benchmark: traversals

Benchmark                       traversals-4.19.2  traversals-master            traversals-after          
bytestring/imap/bytes                     5.41e-5            5.53e-5    +2.17%           5.44e-5    +0.56%
bytestring/map/each                       4.70e-5            3.28e-5   -30.28%           3.32e-5   -29.51%
bytestring/map/native                     5.02e-4            4.76e-4    -5.11%           4.79e-4    -4.47%
hash map/imap/imap                        8.04e-4            7.29e-4    -9.33%           8.08e-4    +0.55%
hash map/imap/native                      3.58e-4            3.76e-4    +4.98%           3.58e-4    +0.02%
hash map/map/each                         7.08e-4            7.03e-4    -0.75%           7.32e-4    +3.35%
hash map/map/native                       3.47e-4            3.55e-4    +2.31%           3.59e-4    +3.59%
list/imap/imap                            1.41e-4            1.24e-4   -11.98%           9.83e-5   -30.28%
list/map/each                             1.07e-4            9.70e-5    -9.48%           9.18e-5   -14.34%
list/map/native                           1.08e-4            9.27e-5   -14.40%           9.27e-5   -14.42%
map/imap/each                             5.34e-4            3.94e-4   -26.27%           5.31e-4    -0.49%
map/imap/native                           5.38e-4            5.28e-4    -1.77%           5.28e-4    -1.74%
map/map/each                              4.00e-4            3.87e-4    -3.16%           3.98e-4    -0.44%
map/map/itraversed                        3.97e-4            3.92e-4    -1.32%           4.01e-4    +0.88%
map/map/native                            4.01e-4            3.91e-4    -2.55%           3.99e-4    -0.53%
sequence/imap/imap                        2.35e-4            2.33e-4    -0.50%           2.18e-4    -7.19%
sequence/imap/native                      2.32e-4            2.30e-4    -0.91%           2.15e-4    -7.46%
sequence/map/each                         2.07e-4            2.04e-4    -1.29%           2.03e-4    -1.61%
sequence/map/native                       2.06e-4            2.05e-4    -0.84%           2.02e-4    -2.02%
unboxed-vector/imap/itraversed            7.17e-5            7.21e-5    +0.47%           7.10e-5    -1.02%
unboxed-vector/imap/native                7.98e-5            7.79e-5    -2.37%           7.85e-5    -1.69%
unboxed-vector/map/itraversed             7.22e-5            6.94e-5    -3.81%           7.09e-5    -1.77%
unboxed-vector/map/native                 7.22e-5            7.13e-5    -1.27%           7.10e-5    -1.70%
vector/imap/imap                          9.43e-5            9.39e-5    -0.43%           1.01e-4    +6.78%
vector/imap/itraversed                    9.66e-5            5.03e-4  +421.16%           3.68e-4  +280.86%
vector/imap/native                        9.53e-5            9.37e-5    -1.75%           9.90e-5    +3.80%
vector/map/itraversed                     8.75e-5            1.60e-4   +83.17%           1.70e-4   +94.31%
vector/map/native                         8.23e-5            8.46e-5    +2.81%           9.99e-5   +21.39%
zzz-geom-mean                             1.79e-4            1.85e-4    +3.46%           1.86e-4    +3.77%

Why the jump in between? Not sure.

map/imap/each                             5.34e-4            3.94e-4   -26.27%           5.31e-4    -0.49%

Vector seems regressed, have to check if this is RULES not firing.

vector/imap/imap                          9.43e-5            9.39e-5    -0.43%           1.01e-4    +6.78%
vector/imap/itraversed                    9.66e-5            5.03e-4  +421.16%           3.68e-4  +280.86%
vector/imap/native                        9.53e-5            9.37e-5    -1.75%           9.90e-5    +3.80%
vector/map/itraversed                     8.75e-5            1.60e-4   +83.17%           1.70e-4   +94.31%
vector/map/native                         8.23e-5            8.46e-5    +2.81%           9.99e-5   +21.39%

benchmark: folds

Benchmark                         folds-4.19.2  folds-master            folds-after          
bytestring/itoList/bytes               7.68e-5       7.63e-5    -0.64%      7.70e-5    +0.17%
bytestring/itoList/native              9.48e-5       9.41e-5    -0.73%      9.33e-5    -1.59%
bytestring/toList/bytes                6.07e-5       6.18e-5    +1.70%      6.19e-5    +1.99%
bytestring/toList/each                 6.09e-5       6.15e-5    +1.10%      6.16e-5    +1.26%
bytestring/toList/native               4.28e-5       4.26e-5    -0.46%      4.26e-5    -0.54%
hash map/itoList/itoList               1.96e-4       2.06e-4    +5.12%      1.14e-4   -41.75%
hash map/itoList/itraversed            2.16e-4       2.04e-4    -5.61%      2.01e-4    -6.93%
hash map/itoList/native                1.16e-4       1.14e-4    -1.95%      1.22e-4    +4.53%
hash map/sum/each                      6.49e-4       5.50e-4   -15.30%      5.95e-4    -8.41%
hash map/sum/native                    5.49e-4       5.35e-4    -2.52%      5.32e-4    -3.04%
hash map/toList/each                   2.61e-4       1.81e-4   -30.65%      1.93e-4   -26.11%
hash map/toList/native                 1.10e-4       1.03e-4    -6.75%      1.00e-4    -9.11%
list/itoList/itraversed                7.36e-5       1.28e-4   +73.83%      3.11e-4  +321.76%
list/itoList/native                    7.36e-5       7.27e-5    -1.21%      7.31e-5    -0.57%
list/toList/each                       5.70e-5       5.64e-5    -0.98%      5.62e-5    -1.33%
list/toList/native                     3.34e-5       3.34e-5    +0.00%      3.34e-5    +0.01%
map/itoList/itraversed                 2.20e-4       2.03e-4    -7.72%      2.00e-4    -9.40%
map/itoList/native                     1.85e-4       1.80e-4    -3.15%      1.76e-4    -4.99%
map/toList/each                        1.09e-4       1.10e-4    +1.13%      9.69e-5   -10.76%
map/toList/native                      9.37e-5       9.19e-5    -1.90%      8.92e-5    -4.87%
sequence/itoList/itraversed            1.28e-4       1.12e-3  +773.63%      1.01e-3  +688.80%
sequence/itoList/native                2.38e-4       2.20e-4    -7.88%      2.11e-4   -11.37%
sequence/toList/each                   6.82e-5       6.55e-5    -4.03%      6.47e-5    -5.10%
sequence/toList/native                 8.68e-5       8.80e-5    +1.34%      8.75e-5    +0.80%
unboxed-vector/itoList/native          7.54e-5       7.47e-5    -0.82%      7.49e-5    -0.66%
unboxed-vector/itoList/vTraverse       7.51e-5       7.30e-5    -2.81%      7.48e-5    -0.32%
unboxed-vector/toList/each             5.80e-5       5.87e-5    +1.21%      5.86e-5    +0.95%
unboxed-vector/toList/native           5.80e-5       5.82e-5    +0.47%      5.88e-5    +1.50%
vector/itoList/itraversed              7.01e-5       1.31e-4   +86.45%      1.12e-4   +59.09%
vector/itoList/native                  6.88e-5       6.99e-5    +1.53%      6.92e-5    +0.62%
vector/toList/each                     5.50e-5       5.47e-5    -0.48%      5.48e-5    -0.34%
vector/toList/native                   5.34e-5       5.33e-5    -0.23%      5.35e-5    +0.27%
zzz-geom-mean                          1.01e-4       1.09e-4    +7.90%      1.09e-4    +7.85%

The itoList/itraversed combo doesn't seem to work well.

list/itoList/itraversed                7.36e-5       1.28e-4   +73.83%      3.11e-4  +321.76%
sequence/itoList/itraversed            1.28e-4       1.12e-3  +773.63%      1.01e-3  +688.80%
vector/itoList/itraversed              7.01e-5       1.31e-4   +86.45%      1.12e-4   +59.09%

@phadej
Copy link
Collaborator Author

phadej commented Dec 12, 2020

I added more rules and now the benchmarks don't seem to have anything suspicious (my machine is incredibly noisy, I suspect some boosts are firing here and there)

benchmark: folds

Benchmark                         folds-4.19.2  folds-after            folds-rules         

list/itoList/itraversed                7.36e-5      3.11e-4  +321.76%      8.16e-5  +10.81%
sequence/itoList/itraversed            1.28e-4      1.01e-3  +688.80%      1.71e-4  +32.99%
vector/itoList/itraversed              7.01e-5      1.12e-4   +59.09%      6.96e-5   -0.65%

zzz-geom-mean                          1.01e-4      1.09e-4    +7.85%      9.99e-5   -0.76%

benchmark: traversals

Benchmark                       traversals-4.19.2  traversals-after            traversals-rules         

vector/imap/imap                          9.43e-5           1.01e-4    +6.78%           1.08e-4  +14.38%
vector/imap/itraversed                    9.66e-5           3.68e-4  +280.86%           9.61e-5   -0.47%
vector/imap/native                        9.53e-5           9.90e-5    +3.80%           9.41e-5   -1.30%
vector/map/itraversed                     8.75e-5           1.70e-4   +94.31%           8.80e-5   +0.60%
vector/map/native                         8.23e-5           9.99e-5   +21.39%           8.46e-5   +2.77%

zzz-geom-mean                             1.79e-4           1.86e-4    +3.77%           1.85e-4   +3.68%

CHANGELOG.markdown Outdated Show resolved Hide resolved
CHANGELOG.markdown Outdated Show resolved Hide resolved
CHANGELOG.markdown Outdated Show resolved Hide resolved
CHANGELOG.markdown Outdated Show resolved Hide resolved
CHANGELOG.markdown Outdated Show resolved Hide resolved
CHANGELOG.markdown Outdated Show resolved Hide resolved
@phadej
Copy link
Collaborator Author

phadej commented Dec 13, 2020

LOL. Each "commit suggestion" triggers CI. Complete non-sense. (Yes, I noticed Add suggestion to batch after I made three clicks...)

@phadej phadej force-pushed the with-index branch 5 times, most recently from f00f1ab to 2985eaf Compare December 13, 2020 02:14
@phadej phadej changed the title WIP: Use traversable-with-index Use indexed-traversable Dec 13, 2020
@phadej
Copy link
Collaborator Author

phadej commented Dec 13, 2020

Pushed a variant using ekmett/comonad#54

Copy link
Collaborator

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Can you include "Fixes #941" in the commit message so that #941 will auto-close when this lands?

src/Control/Lens/Indexed.hs Outdated Show resolved Hide resolved
CHANGELOG.markdown Outdated Show resolved Hide resolved
CHANGELOG.markdown Outdated Show resolved Hide resolved
@phadej
Copy link
Collaborator Author

phadej commented Dec 16, 2020

Sorry, forgot to include autoclose message. Won't do that, to not torture Travis.

Copy link
Collaborator

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Thanks for helping making this happen!

@phadej
Copy link
Collaborator Author

phadej commented Dec 17, 2020

Should we bump the next version to be 5.0

Reasoning, linear defines WithIndex instances for V2, V3 etc. and uses lens <5 constraint. So this change breaks stuff inside kmettverse.

Technically the re-exporting doesn't break linear, but it makes it have unconsistent API depending on which lens is in the install plan. Note: it's perfectly fine to have lens-4.19 (with own FunctorWithIndex) and indexed-traversable in the same install plan.

I'd make a patch to linear to depend directly on indexed-traversable and define both FunctorWithIndex, for old lens and indexed-traversable.

@phadej phadej deleted the with-index branch December 17, 2020 12:17
@RyanGlScott
Copy link
Collaborator

I'm a bit confused. How exactly is linear broken?

I don't have a strong opinion on what the next version of lens should be, as it will be a major change regardless of whether we pick 4.20 or 5.0. But I don't yet understand the "breaks stuff inside kmettverse" argument.

@phadej
Copy link
Collaborator Author

phadej commented Dec 17, 2020

Imagine a downstream of linear which doesn't depend on lens directly

build-depends: linear, indexed-traversable
import Linear.V2 -- from linear
import Data.Functor.WithIndex  -- from indexed-traversable

--
vector :: V2 Int
vector = imap (someFunction :: E V2 -> () -> Int) (pure ())

now, whether that compiles would depend on lens version:

  • if lens-4.20 (or lens-5), this will work.
  • if it islens-4.19 (current one), it wouldn't!

EDIT: and linear-1.21.3 (current one) has lens <5 bounds. So both scenarios are possible, and somewhat likely, as others than edward packages (should) use lens <4.20 bounds.

@phadej
Copy link
Collaborator Author

phadej commented Dec 17, 2020

So possible action points are either:

  • make revisions to linear to have lens <4.20 bounds
  • or make next lens lens-5

@RyanGlScott
Copy link
Collaborator

OK. But I think I'm still missing something. Supposing that we add restrictive upper version bounds for lens in the package metadata for linear, what happens if a build plan that includes lens-4.19 or earlier is chosen? Presumably, that build plan would also include an old version of linear that doesn't define instances from indexed-traversable. Therefore, the program in #951 (comment) would still fail to compile, right?

@phadej
Copy link
Collaborator Author

phadej commented Dec 17, 2020

Yes it will fail to compile, as it does now.

If we release lens-4.20 now. Then the code above would be possible to write (with existing linear). Then if some extra dependency is added which would constraint lens again, it would break.

TL;DR, the "which FunctorWithIndex (old) linear defines becomes fragile. I don't like. Bumping lens to 5 and having linear release which defines both variants (indexed-traversable for old lens and ) would prevent that. Or revisions, such old linear couldn't be used with new lens.

@RyanGlScott
Copy link
Collaborator

Alright. If I understand you correctly, the problem you are trying to prevent is that you might write code that only imports linear and indexed-traversable, and whether that code compiles or not depends on two factors:

  1. What version of linear you are using, and
  2. What version of lens (a library that linear depends on) you are using.

Currently, (1) and (2) are independent, which means that you could accidentally change (2) without ever changing (1). By imposing appropriately restrictive version bounds, changing (2) (i.e., changing the version of lens) would necessarily change (1) (i.e., change the version of linear). Therefore, it is possible to know whether the program in #951 (comment) compiles or not purely by examining which version of linear is in use, rather than having to know both the linear and lens versions.

Did I get all that right?

@phadej
Copy link
Collaborator Author

phadej commented Dec 17, 2020

Did I get all that right?

Yes.

@RyanGlScott
Copy link
Collaborator

Good to know. Here are some other thoughts:

  1. This affects more than just linear. In the Kmettiverse alone, trifecta and zippers define FunctorWithIndex instances and depend on lens < 5. There are other likely other libraries outside of the Kmett diaspora that have similar problems.
  2. In fact, this probably affects more than just *WithIndex instances. I imagine that Swapped instances could face similar problems.

Given that the Kmett-major version number is such a widely used convention, perhaps we should save ourselves some work and just make the next release be lens-5. We could achieve the same end result by making the next release be lens-4.20 and revising several libraries' upper version bounds accordingly, but it would be nice to avoid that amount of revision churn. (The only downside is that we won't be able to make "4.20-blaze-it" jokes. Oh well.)

@phadej
Copy link
Collaborator Author

phadej commented Dec 17, 2020

Good point about Swapped. I do think that next release shuffles enough stuff that even "marketing-wise", lens-5 is justified.

RyanGlScott added a commit that referenced this pull request Dec 23, 2020
The next `lens` release will migrate to using the
`{Functor,Foldable,Traversable}WithIndex` classes from the
`indexed-traversable` package. As discussed in
#951 (comment), this poses a
challenge for downstream code that use instances of
`{Functor,Foldable,Traversable}WithIndex` from `indexed-traversable` while
also indirectly importing `lens`. This is because the code can succeed or fail
to compile depending on what version of `lens` is picked in the build plan,
even if there isn't a direct dependency declared against `lens`!

The best way to mitigate this possibility is to ensure that packages that
depend on `lens` and define `{Functor,Foldable,Traversable}WithIndex` have
appropriately tight version bounds on `lens`. At the time of writing, the most
recent Hackage release is `lens-4.19.*`, so we could ensure that packages
either declare `< 4.20` or `< 5`. As it turns out, many packages in the
Kmettiverse already declare `< 5`, so in order to avoid making more Hackage
revisions than we absolutely need to, we have decided to just bump the version
number to 5. In other words, we are bumping the "Kmett-major" version number.
It's probably a good time to do this regardless, as there are enough breaking
changes in the next release that it makes sense marketing-wise.
RyanGlScott added a commit that referenced this pull request Dec 23, 2020
The next `lens` release will migrate to using the
`{Functor,Foldable,Traversable}WithIndex` classes from the
`indexed-traversable` package. As discussed in
#951 (comment), this poses a
challenge for downstream code that use instances of
`{Functor,Foldable,Traversable}WithIndex` from `indexed-traversable` while
also indirectly importing `lens`. This is because the code can succeed or fail
to compile depending on what version of `lens` is picked in the build plan,
even if there isn't a direct dependency declared against `lens`!

The best way to mitigate this possibility is to ensure that packages that
depend on `lens` and define `{Functor,Foldable,Traversable}WithIndex` have
appropriately tight version bounds on `lens`. At the time of writing, the most
recent Hackage release is `lens-4.19.*`, so we could ensure that packages
either declare `< 4.20` or `< 5`. As it turns out, many packages in the
Kmettiverse already declare `< 5`, so in order to avoid making more Hackage
revisions than we absolutely need to, we have decided to just bump the version
number to 5. In other words, we are bumping the "Kmett-major" version number.
It's probably a good time to do this regardless, as there are enough breaking
changes in the next release that it makes sense marketing-wise.
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