Opened 7 years ago

Closed 6 years ago

Last modified 2 years ago

#5129 closed bug (fixed)

"evaluate" optimized away

Reported by: dons Owned by: simonmar
Priority: normal Milestone:
Component: Compiler Version: 7.0.3
Keywords: seq, evaluate Cc: ezyang@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D615
Wiki Page:

Description

Reported on Stackoverflow: http://stackoverflow.com/questions/5697159/testing-for-error-calls-in-hunit

With optimizations on, the following program, which normally succeeds (correctly generating an exception), instead fails, and the exception is optimized away.

import Control.Exception
import Test.HUnit

throwIfNegative :: Int -> String
throwIfNegative n | n < 0     = error "negative"
                  | otherwise = "no worries"
{-# NOINLINE throwIfNegative #-}

case_negative =
    handleJust errorCalls (const $ return ()) $ do
        evaluate $ throwIfNegative (-1)
        assertFailure "must throw when given a negative number"
  where errorCalls (ErrorCall _) = Just ()

main = runTestTT $ TestCase case_negative

Looking at the core, after a few iterations, the call to throwIfNegative is dropped as dead code.

Using seq instead of evaluate is happy enough:

case_negative =
    handleJust errorCalls (const $ return ()) $ do
        throwIfNegative (-1) `seq` assertFailure "must throw when given a negative number"
  where errorCalls (ErrorCall _) = Just ()

or a bang pattern:

case_negative =
    handleJust errorCalls (const $ return ()) $ do
        let !x = throwIfNegative (-1)
        assertFailure "must throw when given a negative number"
  where errorCalls (ErrorCall _) = Just ()

Possibly related to #2273

Change History (19)

comment:1 Changed 7 years ago by ezyang

Cc: ezyang@… added

comment:2 Changed 7 years ago by simonpj

Actually this ticket is a bit different to #2273. Here is what is happening.

Here are the background definitions:

evaluate :: a -> IO ()
-- Defined in GHC.IO.Base
evaluate x = x `seq` return ()

poss_err :: String 
-- Assume this doesn't get constant folded away
-- So poss_err isn't definitely bottom.
-- (You had a NOINLINE on throwIfNegative)
poss_err = if (-1) < 0 then error "urk" else "foo"

assertFailure :: String -> IO ()
-- Defined in Test.HUnit.Lang
assertFailure s = throw (userError "blah")

Now consider the term

...catch (evaluate poss_err >> assertFailure "foo")...

By the time we've inlined (>>) and evaluate, we get something like this

...catch (\s. case poss_err of _ -> assertFailure "foo" s)...

Now if GHC sees (case x of y { ...blah... }), and y is used strictly in ...blah..., it discards the case expression. After all, we reason that if x turned out to throw an exception, we'll get the exception later instead. And indeed, since ...blah... diverges (well, throws a different error), y is used strictly.

It's like the case in our imprecise-exception paper where we argue that we should not specify which of the two errors is thrown by (error "urk" + error "bok"). It's imprecise.

However, this is the IO monad, where the order of evaluation is defined, and the exceptions raise are too. There are two mistakes.

  • In GHC.IO.Base, the function evaluate is small and hence can be inlined. That exposes the fact that the state token is not affected by evaluating the argument. If we didn't inline evaluate we'd end up with something like
      \s.  case (evaluate poss_err s) of (# s1, _ #) -> assertFailure "blah" s1
    
    Now we can't drop the case because we need the state token s1. If we just put a {-# NOINLNE #-} on evaluate we'll hide this from GHC so it won't discard such cases.
  • In Test.HUnit.Lang, the function assertFailure has an IO type, but is defined using throw. It should be define using throwIO. The whole point of throwIO is that it consumes a state token, and that's what sequences it relative to earlier producers of the state token.

I'll fix evaluate, but someone else had better deal with HUnit.

Simon

comment:3 Changed 7 years ago by ezyang

It seems to me that we ought to be able to inline evaluate; if we emitted something directly like:

case (# touch s, poss_err #) of (# s1, _ #) -> assertFailure "blah" s1

where touch is some primitive that eventually becomes a no-op but enforces ordering (maybe we don’t even need it as it stands?). In this case the case wouldn't be dropped. NOINLINE works, but it seems a little wrong to me to add the overhead of a function application.

comment:4 Changed 7 years ago by simonpj

No -- GHC would just split them apart. You'd need a new primitive:

evaluate# :: o -> State# a -> State# a

That is, with the same type as touch# but it does evaluation, which touch# does not. Otherwise I can't see how to be certain that the eval will never be dropped; the only way I can see is to combine evaluation and state-token-transformation in a way that GHC can't disentangle, hence evaluate#.

I don't think this is common enough that the performance impact of an out-of-line call will be important.

Simon

comment:5 Changed 7 years ago by simonpj

Owner: set to simonmar

Simon and I have just agreed that the right thing is to implement a small family of primops:

touchS# :: a -> State# b -> State# b     -- The current touch#
seqS#   :: a -> State# b -> State# b     -- Used for 'evaluate'
parS#   :: a -> State# b -> State# b     -- Use for the par monad

comment:6 Changed 7 years ago by ezyang

One rather user-visible result of this ticket is that we should now discourage the use of the idiom

x `seq` return ()

I can see a few instances of this in base

ezyang@javelin:~/Dev/ghc-master/libraries/base$ grep -R '`seq` return ()' .
./Foreign/C/String.hs:        go [] n     = n `seq` return () -- make it strict in n
./Foreign/C/String.hs:        go [] n     = n `seq` return () -- make it strict in n
./GHC/IO/Handle/Text.hs:    c `seq` return ()
./GHC/Conc/Windows.hs:  r `seq` return () -- avoid space leak

and there are probably more elsewhere.

comment:7 Changed 7 years ago by simonpj

Indeed. Every one of these should be call to evaluate; that is what evaluate is for! Ian: can you make it so?

Simon

comment:8 Changed 7 years ago by simonpj

I'm just adding Don's comment and my response from ghc-users.

Don says: that's a very common idiom. Interestingly, we have:

-- | Strict (call-by-value) application, defined in terms of 'seq'.
($!)    :: (a -> b) -> a -> b
#ifdef __GLASGOW_HASKELL__
f $! x  = let !vx = x in f vx  -- see #2273 
#elif !defined(__HUGS__) f 
$! x  = x `seq` f x
#endif

Simon's response: It's very different, I think.

  • evaluate is in the IO monad, and (should) guarantee to evaluate the argument before proceeding, so if evaluating the argument to WHNF diverges or throws a exception, any exceptions thrown (in the IO monad at least) after the 'evaluate' should not happen. For example:
    evaluate (throw "first") >> throwIO (userError "second")
    
    should guaranteed to throw "first" and not "second". (The current bug is the 'evaluate' doesn't meet its guarantee.)
  • Strict application ($!) is not in the IO monad, so it merely makes a strictness guarantee. It doesn't guarantee to make exceptions in the argument "beat" exceptions in the function. For example
    ((:) $! (throw "second")) $! (throw "first")
    
    does not guaranteed to throw "first" rather than "second".

Does that distinction make sense? Perhaps the contrast is a useful one. I wonder where it might be documented?

comment:9 Changed 7 years ago by rl

Why is x `seq` return () discouraged? Why would I use evaluate if I want to make my code strict in x but don't care when it is evaluated? This seems to be what ezyang's examples are about (the ones in Foreign.C.String even say so explicitly) so I don't understand why these should be replaced with evaluate.

comment:10 Changed 7 years ago by simonpj

OK fair enough. If all you want is strictness, then seq should be fine. If you want exception ordering, or space-leak squashing, then evaluate is better.

comment:11 Changed 7 years ago by ezyang

I would like to understand this tradeoff better. I would like to be able to say "use evaluate whenever the IO monad is available" but it seems you might preclude some optimizations with this advice.

comment:12 Changed 7 years ago by simonmar

Right, it's quite common to use

   do ...
      return $! e

to avoid the thunk that would otherwise be created for e. My guess is that in most cases we don't care about the order of evaluation in these cases, so $! is fine.

It also occurs to me that the simplifier won't know how to optimise seqS# unless we teach it. For example, we want to eliminate seqS# when applied to a value.

comment:13 Changed 6 years ago by simonmar

See also Note [Desugaring seq] (1) and (2) in DsUtils.

comment:14 Changed 6 years ago by marlowsd@…

commit be5441799b7d94646dcd4bfea15407883537eaaa
Author: Simon Marlow <marlowsd@gmail.com>
Date:   Mon Jun 27 16:45:15 2011 +0100

    Add two new primops:
    
      seq#   :: a -> State# s -> (# State# s, a #)
      spark# :: a -> State# s -> (# State# s, a #)
    
    seq# is a version of seq that can be used in a State#-passing
    context.  We will use it to implement Control.Exception.evaluate and
    thus fix #5129.  Also we have plans to use it to fix #5262.
    
    spark# is to seq# as par is to pseq.  That is, it creates a spark in a
    State#-passing context.  We will use spark# and seq# to implement rpar
    and rseq respectively in an improved implementation of the Eval monad.

comment:15 Changed 6 years ago by simonmar

Status: newmerge

We now have a test for this, and once that is merged this ticket is done. The remaining issue with seq is tracked in #5262.

commit 5ba6997081dd2cba642e61d21d362d050e388e7a
Author: Simon Marlow <marlowsd@gmail.com>
Date:   Wed Jun 29 15:00:14 2011 +0100

    Add test for #5129

comment:16 Changed 6 years ago by igloo

Resolution: fixed
Status: mergeclosed

Merged

comment:17 Changed 3 years ago by simonpj

A later note, in the light of this email thread. Looking at the Haddock documentation for evaluate, Roman said: we seem to have three possible implementations for evaluate:

1) evaluate x  = return $! x
2) evaluate x  = (return $! x) >>= return
3) evaluate x = \s -> seq# x s

Now (1) is too strict: (evaluate (error "urk") True) should yield True.

And, contrary to my claim in the email thread, (2) suffers from comment:2 above: inlining can mean that the argument to evaluate never gets evaluated.

Consider, under (2)

evaluate = \x -> (return $! x) >>= return
         = \x. \s.  case return $! x s of (s1, r) -> return r s1
         = \x s. x `seq` case (s,x) of (s1, r) -> return r s1
         = \x s. x `seq` (s,x) 
         = \x s. case x of _ -> (s,x)

Now consider the call in comment:2:

evalutate poss_err >> assertFailure "foo"`

= (inline >>)
  \s -> case evaluate poss_err of (s1, _) -> assertFailure "foo" s1

= (inline evaluate)
  \s -> case (case x of _ -> (s,x)) of (s1, _) -> assertFailure "foo" s1

= (case of case)
  case x of _ -> assertFailure "foo" s1

and now, as explained in comment:2, since assertFailure diverges, GHC feel free to discard the case on x.

Using a primop seq# :: a -> State# -> (State#, a) means that the I/O options that follows, which consumes the state that comes out of the seq# simply can't start until the seq# has executed. And that is what we want for evaluate.

So only (3) will do.

The claim in the Haddock docs for evaluate that (2) is a correct definition is, I think, false. Simon M added this claim in commit aa73318a in (base repo). I think I'll simply delete it.

Last edited 3 years ago by simonpj (previous) (diff)

comment:18 Changed 3 years ago by Feuerbach

Thanks Simon. I'll try to digest what you've written, and based on my understanding I'll write a better documentation for evaluate that would explain the issue.

— Roman

Last edited 3 years ago by Feuerbach (previous) (diff)

comment:19 Changed 2 years ago by thomie

Differential Rev(s): Phab:D615

In commit de9a836cd920722a4c28dcb464ff2c8d5905acb9:

Author: Roman Cheplyaka <>
Date:   Mon Feb 9 13:44:03 2015 -0600

    Clarify the documentation for 'evaluate'
    
    Summary:
    See:
    
      https://www.haskell.org/pipermail/ghc-devs/2015-January/007900.html
      https://ghc.haskell.org/trac/ghc/ticket/5129#comment:17
    
    Reviewers: hvr, Mikolaj, austin
    
    Reviewed By: Mikolaj, austin
    
    Subscribers: ezyang, nomeata, thomie
    
    Differential Revision: https://phabricator.haskell.org/D615
Note: See TracTickets for help on using tickets.