Opened 8 years ago

Closed 7 years ago

Last modified 3 years ago

#5205 closed bug (fixed)

Control.Monad.forever leaks space

Reported by: akio Owned by: pcapriotti
Priority: high Milestone: 7.6.1
Component: libraries/base Version: 7.0.3
Keywords: Cc: hackage.haskell.org@…, alexey.skladnoy@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case: T5205
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

The attached program, compiled with GHC 7.0.3, uses up all the memory. It runs in a constant space when compiled with GHC 6.12.3.

Attachments (1)

forever.hs (60 bytes) - added by akio 8 years ago.
test case

Download all attachments as: .zip

Change History (17)

Changed 8 years ago by akio

Attachment: forever.hs added

test case

comment:1 Changed 8 years ago by igloo

Milestone: 7.2.1
Owner: set to igloo
Priority: normalhighest

Thanks for the report. I'll take a look.

comment:2 Changed 8 years ago by liyang

Cc: hackage.haskell.org@… added

comment:3 Changed 8 years ago by simonmar

Owner: changed from igloo to simonpj

Ok, we looked at this, and it turns out that 6.12.3 desugars forever differently: in 6.12, a local recursive let was introduced, which meant that forever could be inlined (and hence specialised) at every call site, whereas in 7.0 the desugarer leaves the function as a top-level recursive function which cannot be inlined.

The solution is to add an INLINABLE pragma for forever, which will allow it to be specialised at a call site.

comment:4 Changed 8 years ago by simonpj

Owner: changed from simonpj to igloo

I've made 'forever' INLINABLE, and added a SPECIALISE pragma in GHC.ST.

commit ae10342b49b95393b09ffee8df8c847409699968
Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Thu Jun 9 20:44:21 2011 +0100

    Make 'forever' inlinable (fixes Trac #5205)
    
    See Note [Make forever INLINABLE] in Control.Monad

>---------------------------------------------------------------

 Control/Monad.hs |   18 ++++++++++++++++++
 GHC/ST.lhs       |    4 ++++
 2 files changed, 22 insertions(+), 0 deletions(-)

That fixes the bug, but only with -O. Without -O you still get the leak, and I don't really think its unreasonable. You have

x = forever (return ())

which expands to

x = return () >> return () >> return () >> ....etc....

If (>>) was expensive when applied to two args, then it'd be right to hang onto the computed result. In the case of IO it isn't expensive, and it's best to recompute (and save space) but only the optimiser can reveal that.

Ian: can you add a perf/ test please, then close?

Simon

comment:5 Changed 8 years ago by igloo

Priority: highestnormal

comment:6 Changed 8 years ago by simonpj

Ian: all we need is a perf test for this, and we can close it.

comment:7 Changed 8 years ago by igloo

Resolution: fixed
Status: newclosed

Test added.

comment:8 Changed 7 years ago by edsko

Owner: igloo deleted
Resolution: fixed
Status: closednew

I have an example with a space leak (according to "+RTS -hy") due to "forever" when compiled with "-prof -fauto-all -O1" and when compiled with "-prof -fprof-auto -O2", but not when compiled with "-prof -fprof-auto -O0" for some bizarre reason. When I remove the "-prof -fprof-auto" and profile with "+RTS -hT" the space leak disappears for any optimization level.

I have tried to minimize the example but failed so far, unfortunately. However, the following alternative definition of "forever" doesn't have a space leak with any compiler flags:

forever a = let a' = a >> a' in a'

Is there a good reason why this definition is not used? It seems less reliant on optimization to avoid the space leak.

comment:9 Changed 7 years ago by simonpj

difficulty: Unknown
Owner: set to igloo

Actually that makes a lot of sense. Ian or Paolo, would you like to change the definition of 'forever' as edsko suggests, and check it's ok with your performance test.

Ian: you say that you added a test, but you didn't say what the test is on this ticket, so maybe you can do that too?

(Thinking about it, I'm really not sure what difference it makes to make the current recursive defn of forever INLINABLE. The comment doesn't explain! But if we change the definition it becomes moot anyway.)

comment:10 Changed 7 years ago by pcapriotti

Owner: changed from igloo to pcapriotti

comment:11 Changed 7 years ago by igloo

Test Case: T5205

comment:12 Changed 7 years ago by Khudyakov

Cc: alexey.skladnoy@… added

comment:13 Changed 7 years ago by simonmar

Milestone: 7.2.17.6.1
Priority: normalhigh

Moving to an active milestone.

comment:14 Changed 7 years ago by pcapriotti

Status: newmerge

Pushed:

commit f55f5574c12ff8dfe57994219eee0702ac8aba2e
Author: Paolo Capriotti <p.capriotti@gmail.com>
Date:   Mon Aug 20 16:35:38 2012 +0100

    Improve definition of forever (#5205)
    
    The previous implementation was:
    
        forever a = a >> forever a
    
    which can create a space leak in some cases, even with optimizations.
    The current implementation:
    
        forever a = let a' = a >> a' in a'
    
    prevents repeated thunk allocations by creating a single thunk for the
    final result, even without optimizations.

I also removed the note and a SPECIALISE pragma in GHC.ST.

comment:15 Changed 7 years ago by pcapriotti

Resolution: fixed
Status: mergeclosed

comment:16 Changed 3 years ago by Ben Gamari <ben@…>

In d398162/ghc:

testsuite: Separate out Windows results for T5205

This test seems to have much different allocation behavior on Windows
and Linux.  Previously we had widened the acceptance window to 7% to
accomodate this, but even this isn't enough any more. Instead of further
widening the window let's just give an expected number for each
platform. Really, this is precisely the issue with our performance
testing model which I've been complaining about in #12758.

Fixes test for #5205 on 64-bit Windows.

Test Plan: Validate on Windows

Reviewers: austin

Subscribers: thomie, Phyx

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

GHC Trac Issues: #5205
Note: See TracTickets for help on using tickets.