Opened 5 years ago

Closed 4 years ago

#10034 closed bug (invalid)

Regression in mapM_ performance

Reported by: dfeuer Owned by: bgamari
Priority: normal Milestone: 8.0.1
Component: Core Libraries Version: 7.10.1-rc2
Keywords: Cc: core-libraries-committee@…, ekmett
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D632
Wiki Page:

Description

The current mapM_ is sometimes worse than the version in 7.8.3. This can be fixed easily by eta-expansion, matching mapM.

Change History (11)

comment:1 Changed 5 years ago by simonpj

Can you offer a reproducible test case, and timing or allocation data?

Simon

comment:2 in reply to:  1 Changed 5 years ago by dfeuer

Replying to simonpj:

Can you offer a reproducible test case, and timing or allocation data?

I had one, and then I lost it. I wish I could give you one as a test case and for future analysis. It was something very simple, and the mapM_ of 4.7 was consistently the same as the version in the linked differential, and both allocated less than the version currently in head. I believe it had to be an arity issue. Call arity wasn't catching it for some reason, so it needed input from the other arity analysis that it wasn't getting. Consider the current definition:

mapM_ f = foldr ((>>) . f) (return ())

Taking apart the argument to foldr above gives

mapM_ f = foldr (\m -> (>>) (f m)) (return ())

Now fuse this with build g:

mapM_ f (build g) = g (\m -> (>>) (f m)) (return ())

Without knowing what the monad is, mapM_ doesn't know that (>>) has arity 2, and relies on Call Arity analysis of g, which I imagine wasn't working in whatever example I was dealing with. What I propose below forces the arity up to 2, and is also significantly easier to read.

{-# INLINE mapM_ #-}
mapM_ f = foldr (\m n -> f m >> n) (return ())

comment:3 Changed 5 years ago by simonpj

I still don't follow from comment:2 where the performance loss you describe is coming from, nor why inlining mapM will help.

If you do have a clear idea, can you construct an example? Switch off Call Arity if you like.

comment:4 Changed 5 years ago by Austin Seipp <austin@…>

In 7cf87fc6928f0252d9f61719e2344e6c69237079/ghc:

Eta-expand argument to foldr in mapM_ for []

Summary:
This improves performance, at least sometimes--the previous
implementation can be worse than the version in base 4.7. I
have not had the time to run benchmarks and such, but `mapM`
already does this.

Also, inline `mapM_`, like `mapM`.

Reviewers: hvr, nomeata, ekmett, austin

Reviewed By: ekmett, austin

Subscribers: thomie

Projects: #ghc

Differential Revision: https://phabricator.haskell.org/D632

GHC Trac Issues: #10034

comment:5 Changed 5 years ago by simonpj

Priority: normalhighest

I'm very unhappy with applying this patch without any comments, and without any understanding of what is going on. Now mapM_ has an INLINE pragma that may or may not be necessary, and an eta-expanded argument, and no one understands which of the two is important, if either. Maybe there is a simplifier bug underlying this, which is simply being covered up.

Please please can we have a reproducible test case that shows the regression. Ideally one that just showed it, but if it's necessary to switch Call Arity off (which you hypothesise is making the test case harder to create) then do that.

But please let's NOT just cover up the problem. I'm quite inclined to revert the patch, just to serve as a forcing function to try to elicit this information. For now I'll just increase the priority to try to make sure we notice it.

I know it's a nuisance, but this is an example of what people mean by incurring technical debt.

Thanks!

Simon

comment:6 Changed 5 years ago by Austin Seipp <austin@…>

In 91d9530525803403c3c012901115d54ff4fc3b5e/ghc:

Revert "Eta-expand argument to foldr in mapM_ for []"

This change lacked justification (or a test!) for its improvements, and
I merged it on a sweep of Phabricator without fixing this. Trac #10034.

This reverts commit 7cf87fc6928f0252d9f61719e2344e6c69237079.

comment:7 Changed 5 years ago by thoughtpolice

Milestone: 7.10.17.12.1
Priority: highestnormal

Moving out to 7.12 for now pending a better test case/example of where this is going wrong.

comment:8 Changed 4 years ago by bgamari

Owner: changed from ekmett to bgamari

comment:9 Changed 4 years ago by bgamari

David, I don't suppose you have stumbled upon an example of this in the last few months, have you?

Last edited 4 years ago by bgamari (previous) (diff)

comment:10 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:11 Changed 4 years ago by bgamari

Cc: ekmett added
Resolution: invalid
Status: newclosed

Closing as we don't have an example demonstrating the issue. David, feel free to reopen if you do come upon a testcase.

Note: See TracTickets for help on using tickets.