Add forM #592
Add forM #592
Conversation
|
It seems a bit odd to offer the flipped version of traverseSet :: (Ord b, Applicative f) => (a -> f b) -> Set a -> f (Set b)
traverseSet f = fmap fromListN . traverse f . toListI'm not sure whether we can do better than that. The monadic version should probably look like this: mapMSet :: (Ord b, Monad m) => (a -> m b) -> Set a -> m (Set b)
mapMSet f s0 = foldr go return s0 empty
where
go x r s = f x >>= \y -> r $! insert y sor alternatively perhaps mapMSet :: (Ord b, Monad m) => (a -> m b) -> Set a -> m (Set b)
mapMSet f = foldM go empty
where
go s x = f x >>= \y -> pure $! insert y sNote: you should definitely email the libraries list with a formal proposal; in particular, naming these things can be a tad tricky. |
|
I cannot really judge what is more efficient or more performant, since reasoning about those things are not easy in haskell. |
|
The monadic versions I propose insert into the accumulator set as actions are performed, which will likely be considerably more efficient in many cases. Do you understand why the |
|
Ah, I see now that you've written some very similar code yourself, in http://hackage.haskell.org/package/hpath-0.9.2/docs/src/System.Posix.Directory.Traversals.html#traverseDirectory |
Yes, becauce However, if we never care about the Set, but only about the actions, then foldr would be more "efficient" no?
I'm not sure what you mean by that. |
|
ping |
|
You still haven't come up with an implementation I consider reasonable. |
hasufell
force-pushed
the
forM
branch
2 times, most recently
from
4b996f0 to
ff01a54
Compare
|
Much better. But you'll still need to propose this on the libraries list to get community input. |
I did |
|
Can you link to the discussion thread here? Unfortunately I don't remember it. |
Actually, not I did, but someone else: https://mail.haskell.org/pipermail/libraries/2019-January/029335.html |
|
I'm also interested in benchmarks and will test a bit I think. |
|
Ah, yes, that "someone else" was actually me. But there wasn't any real discussion, aside from one comment by Henning. Could you try to reignite the conversation? |
|
I compared forM :: (Ord b, Monad m) => Set a -> (a -> m b) -> m (Set b)
forM s0 f = fmap (GHCExts.fromListN (size s0)) . traverse f . toList $ s0with forM :: (Ord b, Monad m) => Set a -> (a -> m b) -> m (Set b)
forM s0 f = foldr go return s0 empty
where
go x r s = f x >>= \y -> r $! insert y sand forM :: (Ord b, Monad m) => Set a -> (a -> m b) -> m (Set b)
forM s0 f = foldM go empty $ s0
where
go s x = f x >>= \y -> pure $! insert y stest was: bench "forM" $ whnf (\s -> runIdentity $ S.forM s (\e -> pure $ e + 1)) stoList version: foldr version: foldM version: |
Given the disinterest, does this justify two threads? |
|
I find the benchmark results interesting and confusing. |
|
What did your actual benchmark look like?
…On Tue, Jan 7, 2020, 10:40 AM Julian Ospald ***@***.***> wrote:
I find the benchmark results interesting and confusing.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#592?email_source=notifications&email_token=AAOOF7IPCECQWP76D6MTI4TQ4SO47A5CNFSM4GQRXBCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIJIURA#issuecomment-571640388>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOOF7P3K7SY2SJ37B6D4BLQ4SO47ANCNFSM4GQRXBCA>
.
|
|
|
You're probably running into the weird special-case optimization in |
makes the foldr version go faster: fromList one is still the fastest. |
|
|
|
My opinion on this PR:
|
Thanks, will adjust!
I did that, because they have different performance characteristics I think. It appears the applicative version is faster. See above. But I don't know in which circumstances. I guess it makes sense to unify those.
I prefer consistency here. |
|
@andreasabel, could you do me a favor and keep the discussion of what to include and by what name to the libraries list? It's useful to have it all in one place and to get more exposure to comments from other users. |
containers/src/Data/Set/Internal.hs
Outdated
| -- Like 'traverse' from Data.Traversable. This is less generic, since 'Set' | ||
| -- does not have a Traversable instance. | ||
| traverse :: (Ord b, Applicative m) => (a -> m b) -> Set a -> m (Set b) | ||
| traverse f s0 = fmap (GHCExts.fromListN (size s0)) . DT.traverse f . toList $ s0 |
There was a problem hiding this comment.
I know there are some subtle differences on whether to use traverse f s0 vs traverse f, e.g. inlining. What's the verdict here? Do we want it half-pointfree or fully pointfree even?
|
@hasufell, you should bring back the previous |
In fact, this breaks ancient GHC versions where Monad is not a superclass of Applicative: |
|
@treeowl the results are when keeping the two different implementations. This looks significant. |
|
Please let me see your |
ST is already included. |
|
It looks like your |
|
So how did the ordering change affect the benchmarks? |
Not at all |
|
Okay, so for now we don't need to have |
|
Actually I was lying. The new benchmarks ran with the unified implementation. Here are the benchmarks for the explicit foldr ( I can't make any sense out of this. Why the this was with: bench "forM (Identity)" $ nf (\s -> runIdentity $ S.forM s (\e -> pure $ e + 1)) s
, bench "for (Identity)" $ nf (\s -> runIdentity $ S.for s (\e -> pure $ e + 1)) s
, bench "forM (ST)" $ nf (\s -> runST $ do
ref <- newSTRef 0
S.forM s (\e -> do
modifySTRef ref (+ 1)
x <- readSTRef ref
pure $ x + e * 36 `mod` 11)) (S.map (`mod` 51) s)
, bench "for (ST)" $ nf (\s -> runST $ do
ref <- newSTRef 0
S.for s (\e -> do
modifySTRef ref (+ 1)
x <- readSTRef ref
pure $ x + e * 36 `mod` 11)) (S.map (`mod` 51) s) |
|
I noticed when I change the test implementation to: bench "forM (Identity)" $ nf (\s -> runIdentity $ S.forM s (\e -> pure $ e + 1)) s
, bench "for (Identity)" $ nf (\s -> runIdentity $ S.for s (\e -> pure $ e + 1)) s
, bench "forM (ST)" $ nf (\s -> runST $ do
ref <- newSTRef 0
S.forM s (\e -> do
modifySTRef ref (+ 1)
x <- readSTRef ref
pure $ x + e * 36 `mod` 11)) s
, bench "for (ST)" $ nf (\s -> runST $ do
ref <- newSTRef 0
S.for s (\e -> do
modifySTRef ref (+ 1)
x <- readSTRef ref
pure $ x + e * 36 `mod` 11)) sThen the results are: It seems small changes to the tests have significant impact. But the forM and for seem to converge at some point. |
I think it's better to have all 4 functions. Not my decision though. |
|
If it doesn't add functionality and doesn't add performance, then there's a really high bar to adding it. Generally speaking, this is because it makes something really important significantly more convenient (like |
| @@ -136,6 +136,12 @@ module Data.Set ( | |||
| -- * Map | |||
| , S.map | |||
| , mapMonotonic | |||
| #if __GLASGOW_HASKELL__ >= 710 | |||
There was a problem hiding this comment.
This is quite confusing and annoying; please avoid APIs exporting things conditional on GHC or base versions
There was a problem hiding this comment.
@hvr, don't worry; I won't let it through like that. Please join the discussion on the libraries list about whether to add some of these.
There was a problem hiding this comment.
@hvr there are two problems with that. 7.6 and 7.8 don't have Applicative constraint on Monad class and 7.6 does not have fromListN at all, which would result in two different implementations of the same function having different performance characteristics. How do you propose to solve it?
|
No actionable input. I've lost interest in this PR. |
|
@treeowl Since you apparently don't want to let this die, I've assigned it to you. :) |
|
This is dead. Please open your own PR if you're interested to continue. I'm cleaning up stale PRs every once in a while. |
I'm not entirely sure if
forM s f = fmap fromList . (flip T.forM f) . toList $ swould be faster.