Opened 3 years ago

Last modified 17 months ago

#13380 new bug

raiseIO# result looks wrong

Reported by: dfeuer Owned by: dfeuer
Priority: high Milestone:
Component: Compiler Version: 8.1
Keywords: Exceptions Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Other Test Case:
Blocked By: Blocking:
Related Tickets: #14998 Differential Rev(s):
Wiki Page:

Description

primops.txt.pp includes the following note:

raiseIO# needs to be a primop, because exceptions in the IO monad must be precise - we don't want the strictness analyser turning one kind of bottom into another, as it is allowed to do in pure code.

But we do want to know that it returns bottom after being applied to two arguments, so that this function is strict in y

f x y | x>0       = raiseIO blah
      | y>0       = return 1
      | otherwise = return 2

I believe the "But we do" portion is, unfortunately, entirely wrong. From a user perspective, it is extremely strange to replace a precise exception with an imprecise one. That is, I strongly believe the above code should be considered lazy in y. The function f calculates an IO action; the value of y is only necessary to calculate that action if x is non-positive.

We can, conservatively, say for certain that the second component of the result of raiseIO# (i.e., the "return value") is bottom. We can also say, given

case raiseIO# e s of
  (# s', r #) -> q

that q is dead (we could replace it by (# s', undefined #)).

Change History (16)

comment:1 Changed 3 years ago by dfeuer

I've set an optimistic milestone for this, but we will probably need to wait for 8.4 to dig a bit deeper into how demand analysis interacts with raise#, raiseIO#, retry#, catch#, catchSTM#, and catchRetry#. There seem to be subtle problems in multiple places.

comment:2 Changed 3 years ago by rwbarton

I think it's just a bit of imprecise wording. It's right that raiseIO# returns bottom after being applied to two arguments, as:

raiseIO# :: a -> State# RealWorld -> (# State# RealWorld, b #)

But the function in the example is strict in y only after being applied to a third argument, i.e., executed. You can try this:

import Control.Exception

f :: Int -> Int -> IO Int
f x y | x > 0 = throwIO StackOverflow
      | y > 0 = return 1
      | otherwise = return 2

u :: Int
u = -1
{-# NOINLINE u #-}

main :: IO ()
main = f u undefined `seq` return ()

and it's not optimized to bottom.

comment:3 Changed 3 years ago by dfeuer

@rwbarton, no, I understood that just fine. The trouble is that I don't think it's valid to analyze that as bottom when applied to three arguments, because then we lose the perfectly well-defined action of throwing a precise exception. Instead of performing that action, we can end up throwing an imprecise exception evaluating something we have no business evaluating.

comment:4 Changed 3 years ago by dfeuer

Full test case:

import Control.Exception

{-# NOINLINE f #-}
f :: Int -> Int -> IO Int
f x y | x>0       = throwIO (userError "What")
      | y>0       = return 1
      | otherwise = return 2

main = f 2 undefined >>= print

I would expect this to throw the "What" exception, but when it's compiled with optimization I get the "undefined" exception. If I had written throw (userError "What"), that would be acceptable, because we don't make any guarantees about which imprecise exception in the imprecise exception set we actually throw. But here we are re-ordering evaluation in such a fashion as to throw an imprecise exception instead of a precise one, and I don't think that's legit.

comment:5 Changed 3 years ago by dfeuer

Keywords: Exceptions added

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

In 7b087ae/ghc:

Make raiseIO# produce topRes

Make `raiseIO#` produce `topRes` instead of `ExnRes`. `ExnRes` leads to
demand analysis being too aggressive, IMO, allowing imprecise exceptions
produced by `throw` to replace exceptions thrown by `throwIO` that
would like to think of as precise.

This fixes that, but is certanly much more conservative than we would
ideally like. Let's see how bad it is.

Fixes Trac #13380

Reviewers: austin, bgamari

Subscribers: rwbarton, thomie

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

comment:7 Changed 3 years ago by bgamari

Resolution: fixed
Status: newclosed

This is now fixed, although as acknowledged in the commit message we may be able to do better in the future.

comment:8 Changed 3 years ago by simonpj

Wow. We said "let's see how bad it is". Did we measure the perf impact, if any? What's the nofib change?

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

comment:9 in reply to:  8 Changed 3 years ago by dfeuer

Replying to simonpj:

Wow. We said "let's see how bad it is". Did we measure the perf impact, if any? What's the nofib change?

It wasn't absolutely horrifying. /da4687f63ffe5a6162e3d7856aa53de048dd0f42 I'm benchmarking a change right now to manually strictify a few library functions that were likely affected; we'll see how that goes. BTW, did you see my note on the insufficiency of the note on the I/O hack in the demand analyzer?

comment:10 Changed 3 years ago by simonpj

OK: please put the nfib results here.

And check that we still get the improvements reported in #11222

comment:11 Changed 3 years ago by simonpj

Priority: normalhigh
Resolution: fixed
Status: closednew

Re-opening to ensure that we do check those perf numbers.

comment:12 Changed 2 years ago by bgamari

Owner: set to dfeuer

David, could you provide some more quantitative performance evaluation?

comment:13 Changed 2 years ago by dfeuer

Oh, it seems I pasted the link above wrong. I meant to paste a link to the perf dashboard for that commit. There were two significant changes in nofib runtimes.

test                    old     change  new
nofib/time/k-nucleotide 5.338   + 4.95% 5.602 seconds
nofib/time/lambda       1.095   - 3.2%  1.06  seconds

There was also an unfortunate increase in nofib sizes that I didn't notice before (gipeda did not flag it for some reason). The sizes went up by a little over 1% across the board. Ouch. I'm guessing this is because we lost the dead code elimination we previously had for

case raiseIO# e s of ...

Could we recover that by using some sort of special rule somewhere that rewrites

case raiseIO# e s of
  p -> ...

to raiseIO# e s? I could give it a shot if someone tells me where to put it.

There were a few test suite allocation regressions, the largest of which were

tests/alloc/T10547   33561264  + 1.36%  34019072   bytes
tests/alloc/T5837    51419384  + 1.27%  52074608   bytes
tests/alloc/T12425   134735944 + 0.52%  135441336  bytes
tests/alloc/T10858   267909584 + 0.49%  269215800  bytes
tests/alloc/T13035   93847840  + 0.44%  94264440   bytes
tests/alloc/T6048    109461504 + 0.38%  109873560  bytes
tests/alloc/T13056   420744984 + 0.37%  422320424  bytes

I believe the right way to fix these is to go through any code in base that uses throwIO and make sure we manually force everything we want to.

comment:14 Changed 2 years ago by bgamari

Milestone: 8.2.18.4.1

Given the adverse performance effects of comment:6 we have reverted it on ghc-8.2.

comment:15 Changed 20 months ago by bgamari

Milestone: 8.4.1

It's unlikely that this will be fixed for 8.4.

comment:16 Changed 17 months ago by sgraf

Note: See TracTickets for help on using tickets.