#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: new Priority: normal | Milestone: 7.10.2 Component: libraries/base | Version: 7.10.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: None/Unknown | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: -------------------------------------+------------------------------------- Comment (by dolio): `mapM_` is in a similar situation, despite not being in the `Foldable` class. It is currently implemented with `(>>)`, but it could be implemented with `(<*)` or just as `mapM_ = traverse_`. This is desired as well (see the most recent message in the libraries thread at the time of this comment). -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:1> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler

#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: new Priority: normal | Milestone: 7.10.2 Component: libraries/base | Version: 7.10.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: None/Unknown | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: Phab:D924 -------------------------------------+------------------------------------- Changes (by hvr): * differential: => Phab:D924 Comment: here's a first attempt at a patch; please review if that's the change you suggest -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:3> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler

#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: new Priority: normal | Milestone: 7.10.2 Component: libraries/base | Version: 7.10.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: None/Unknown | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: Phab:D924 -------------------------------------+------------------------------------- Comment (by simonpj): Well someone should preferably just check that whatever fusion used to happen still happens, and ideally add a performance test to make sure it stays that way. That done, go ahead if the Core Libraries Committee is happy. (And Edward is presumably speaking as its chair in comment:4.) I'm unconvinced it's worth merging to 7.10. After all, it changes nothing except omitting one line of code from a library, right? Simon -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:5> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler

#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: new Priority: normal | Milestone: 7.10.2 Component: libraries/base | Version: 7.10.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: None/Unknown | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: Phab:D924 -------------------------------------+------------------------------------- Comment (by dolio): I'm not really attached to this, so if simonmar is okay with postponing to 7.12, it's fine with me. Anyone who uses `Monad`s with `(<*>) /= ap` (at least, in ways giving distinct answers) is going to have a bad time, though. The specification of `Monad` now says they need to be equivalent, and I think that gives library writers license to switch between them without making a major version change. And it's only going to get worse. When ApplicativeDo gets implemented, using that extension will silently switch between these things. For instance, if ApplicativeDo were on in the file with the custom `mapM`, it would already be the same as `traverse`, I think. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:7> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler

#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: new Priority: normal | Milestone: 7.10.2 Component: libraries/base | Version: 7.10.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: None/Unknown | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: Phab:D924 -------------------------------------+------------------------------------- Comment (by dolio): If I understand correctly, simonmar has a `Monad` whose `Applicative` instance does batching of remote calls, because you can do that when you use `(<*>)` but not `(>>=)`. So: {{{ f <*> x }}} does one round trip to the server, while: {{{ f >>= \g -> x >>= \y -> return (g y) }}} does two. The custom `mapM` does n trips, where n is the length of the list, while `traverse` does one, because the latter is implemented using: {{{ cons_f x ys = (:) <$> f x <*> ys }}} whereas the custom one is: {{{ cons_f x ys = do { z <- f x ; zs <- ys ; return (z:zs) } }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:9> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler

#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: patch Priority: normal | Milestone: 7.10.2 Component: libraries/base | Version: 7.10.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: None/Unknown | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: Phab:D924 -------------------------------------+------------------------------------- Changes (by thoughtpolice): * status: new => patch -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:11> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler

#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: patch Priority: normal | Milestone: 7.12.1 Component: libraries/base | Version: 7.10.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Revisions: Phab:D1124 -------------------------------------+------------------------------------- Changes (by bgamari): * differential: Phab:D924 => Phab:D1124 Old description:

Recently, Simon Marlow asked why the list instance for Traversable had a custom mapM implementation that used the Monad operations. Having looked a bit, I don't think there's any good reason. The only fusion that the custom mapM can participate in is due to it being written as foldr, but traverse is, as well. So as long as 'mapM = traverse' is able to inline appropriately, there should be no difference.

Further, this can be changed, in principle, for 7.10.2. It doesn't change any types, only the implementation.

mapM = traverse is the class default definition, so this could possibly be completed by just removing the custom definition.

Link to the libraries thread: https://mail.haskell.org/pipermail/libraries/2015-May/025708.html

New description: Recently, Simon Marlow asked why the list instance for `Traversable` had a custom `mapM` implementation that used the `Monad` operations. Having looked a bit, I don't think there's any good reason. The only fusion that the custom `mapM` can participate in is due to it being written as `foldr`, but `traverse` is, as well. So as long as `mapM = traverse` is able to inline appropriately, there should be no difference. Further, this can be changed, in principle, for 7.10.2. It doesn't change any types, only the implementation. `mapM = traverse` is the class default definition, so this could possibly be completed by just removing the custom definition. Link to the libraries thread: https://mail.haskell.org/pipermail/libraries/2015-May/025708.html -- Comment: Phab:D924 proposed a redefinition of `mapM_` in addition to the removal of list's override of `mapM`. This changed the performance characteristics of user code, as seen in #10711. I've extracted the removal of the `mapM` override into Phab:D1124 and forwarded the `mapM_` issue to the Core Libraries Committee. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:13> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler

#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: closed Priority: normal | Milestone: 7.12.1 Component: libraries/base | Version: 7.10.1 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Revisions: Phab:D1124 -------------------------------------+------------------------------------- Changes (by bgamari): * status: patch => closed * resolution: => fixed -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:15> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler

#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: new Priority: normal | Milestone: 8.0.1 Component: libraries/base | Version: 7.10.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Runtime | Unknown/Multiple performance bug | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1124 -------------------------------------+------------------------------------- Changes (by thomie): * failure: None/Unknown => Runtime performance bug Comment: nofib/allocs/cryptarithm2 13094088 + 64.55% 21545696 bytes -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:18> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler

#10457: Revise/remove custom mapM implementation for lists -------------------------------------+------------------------------------- Reporter: dolio | Owner: Type: task | Status: closed Priority: normal | Milestone: 8.0.1 Component: libraries/base | Version: 7.10.1 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Runtime | Unknown/Multiple performance bug | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D1124 Wiki Page: | -------------------------------------+------------------------------------- Comment (by nomeata): JFTR: This instance of the regression has been fixed, because the `State` implementation got a sensible `Applicative` instance. The change in changeset:60297486/ghc still has the ability to regress any user code that has `(<*>) = ap`. OTOH, there is little we can do about it besides telling people to implement their Applicatives properly. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/10457#comment:20> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler