Opened 3 years ago

Closed 3 years ago

Last modified 18 months ago

#13330 closed bug (fixed)

forkIO has inconsistent behavior under optimization

Reported by: dfeuer Owned by:
Priority: normal Milestone: 8.2.1
Component: Core Libraries Version: 8.1
Keywords: Exceptions Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Other Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D3189
Wiki Page:

Description

Troubleshooting the T4030 failure in SimonPJ's early-inline branch, Reid and I discovered that the problem showed up when the argument to forkIO was obviously bottom. He came up with a tiny test case that fails without Simon's changes:

main = forkIO undefined >> threadDelay 1000000

It looks like the trouble is in the definition of forkIO:

forkIO :: IO () -> IO ThreadId
forkIO action = IO $ \ s ->
   case (fork# action_plus s) of (# s1, tid #) -> (# s1, ThreadId tid #)
 where
  action_plus = catchException action childHandler

This seems to run into the trouble with catchException and strictness explained in GHC.IO. It would appear that the conservative fix would be to replace catchException with catch. Personally, I find it a bit surprising that forkIO doesn't force its IO argument before forking, but changing that behavior could break working code.

Change History (22)

comment:1 Changed 3 years ago by rwbarton

To be specific, the expected behavior of the test is for the undefined exception to be reported and then for the program to run for a second and then exit.

This is the behavior without optimizations, and also the behavior under GHC 7.10 and earlier regardless of optimization level. Basically, the same situation as with catch in #11555.

comment:2 Changed 3 years ago by dfeuer

Differential Rev(s): Phab:D3189
Owner: set to dfeuer

comment:3 Changed 3 years ago by dfeuer

Status: newpatch

comment:4 Changed 3 years ago by simonpj

Strange. The definition of catch is just

catch act = catchException (lazy act)

for reasons extensively documented in #11555. So catchException has different, less desirable, and undocumented behaviour.

Should move the lazy into catchException? So then catch = catchException. Do we actually want the distinction?

Incidentally, the use of 'lazy' in catch is entirely un-documented. No Note no nothing. While fixing this ticket it would be good to fix that too. At every least a reference to #11555!

comment:5 in reply to:  4 ; Changed 3 years ago by dfeuer

Replying to simonpj:

Strange. The definition of catch is just

catch act = catchException (lazy act)

for reasons extensively documented in #11555. So catchException has different, less desirable, and undocumented behaviour.

Should move the lazy into catchException? So then catch = catchException. Do we actually want the distinction?

Incidentally, the use of 'lazy' in catch is entirely un-documented. No Note no nothing. While fixing this ticket it would be good to fix that too. At every least a reference to #11555!

Yes, I can document the lazy in catch. The distinction probably does make sense, although it was documented quite incorrectly. The situation here is a bit tricky: we are trying to deal with the fact that catch# evaluates its argument eagerly, but is nevertheless non-strict in that argument. The current approach is to lie a little and tell the demand analyzer that catch# is strict in its argument. catchException inherits this lie.

To avoid getting caught in a lie, and to avoid fragile case-specific reasoning, the invariant we must maintain is that catchException is never called with an undefined argument. When this invariant holds, the analysis should end up being pretty much correct:

  1. It's safe to evaluate the argument and/or its dependencies early, because it will not fail.
  1. After the catchException runs, its argument will be in WHNF.

An alternative approach that smells more honest to me might be to reveal the fact that catch# is actually lazy in its argument, remove the lazy call from catch, and make catchException actually force its argument. I don't know if that would have any negative performance or other consequences.

comment:6 in reply to:  5 Changed 3 years ago by rwbarton

Replying to dfeuer:

To avoid getting caught in a lie, and to avoid fragile case-specific reasoning, the invariant we must maintain is that catchException is never called with an undefined argument. When this invariant holds, the analysis should end up being pretty much correct:

  1. It's safe to evaluate the argument and/or its dependencies early, because it will not fail.
  1. After the catchException runs, its argument will be in WHNF.

Agreed, though we might also document what could happen if the argument actually is bottom (namely catch# will not catch the exception).

An alternative approach that smells more honest to me might be to reveal the fact that catch# is actually lazy in its argument, remove the lazy call from catch, and make catchException actually force its argument. I don't know if that would have any negative performance or other consequences.

Interesting idea. It would mean an extra seq in the original program, but I think GHC would normally be able to eliminate the seq easily.

If this works it might lead to a simpler design; but not urgent.

comment:7 Changed 3 years ago by simonpj

In a conversation on Monday I think we agreed:

  • We should try changing catch# to be lazy (but still with a C1(d) usage) and see what perf effect, if any, that has.
  • Neil Mitchell was part of the conversation on #11555; see comment:11 there. So let's check with him too.

I think David is leading on this.

comment:8 Changed 3 years ago by dfeuer

I'm working on that this morning. The plan is pretty much as you indicated. I'm building now and will try out the benchmarks when it's done.

comment:9 Changed 3 years ago by dfeuer

It looks to me like the original work behind trying to be extra-clever about catchException demand analysis might be recoverable. Fundamentally, this has very little to do with catch# per se. The primop it suggests is a sort of forceIO :: (State# s -> (# State# s, a)) -> (State# s -> (# State# s, a)). The intuition behind this primop is that it reduces an IO action to something with precisely the form

\s -> case PRIMOP s of (# s', ... #) -> e

(that is, reducing until we know what the first primitive IO action will be)

I don't know if this is actually implementable, but I think it's probably what all that fanciness in Demand.hs was trying to get at.

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

comment:10 Changed 3 years ago by dfeuer

Oh, I guess that's not quite right. It's trying to force everything that would be forced if the action runs to completion, isn't it? I have grave doubts about this being a useful and well-defined idea.

comment:11 Changed 3 years ago by David Feuer <David.Feuer@…>

In 701256df/ghc:

Change catch# demand signature

* Give `catch#` a lazy demand signature, to make it more honest.

* Make `catchException` and `catchAny` force their arguments so they
actually behave as advertised.

* Use `catch` rather than `catchException` in `forkIO`, `forkOn`, and
`forkOS` to avoid losing exceptions.

Fixes #13330

Reviewers: rwbarton, simonpj, simonmar, bgamari, hvr, austin

Subscribers: thomie

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

comment:12 Changed 3 years ago by dfeuer

Resolution: fixed
Status: patchclosed

comment:13 Changed 3 years ago by simonpj

Owner: dfeuer deleted
Resolution: fixed
Status: closednew

This is a tricky area so I'd like to be quite sure I understand. Several things:

  • The $exceptions_and_strictness note in GHC/IO.hs says

While @test1@ will print "it failed", @test2@ will print "uh oh".

Previously it said that test2 was unpredictable. Are we saying that both are now predictable? That's good but why?

  • The note does not say that the only difference between catchException and catch is that the former is strict in the action. Indeed, it'd be better to make that point in the doc for catchException itself; it's the only difference between the two.

Since this strictness is a small optimization and may lead to surprising results, all of the @catch@ and @handle@ variants offered by "Control.Exception" use 'catch' rather than 'catchException'.

  • But the comment is confusing. If catchException is more efficient (and we might want to say why) should we not use it all the time in the libraries if we can? What are the "surprising results" that you have in mind?
  • Moreover, specifically what "handle variants" do you have in mind? handle and handleJust? But also catchJust.

Finally, does this efficiency matter? What is the nofib perf loss if we use catchException = catch? And if there is some, can we see a poster-child example so we can really understand when it's important?

comment:14 Changed 3 years ago by dfeuer

Simon, I definitely think more needs to be done in this area. I'm not yet convinced that catchException is really useful at the moment; we should probably just use catch, and force exactly what we need to. I am also concerned about catchRetry# and catchSTM# (see #13357).

However, I just realized that we probably can do something a bit more interesting with catch. In particular, something definitely used by both the action and the recovery function can safely be forced in advance. So if we see, for example,

f x = (x `seq` ...) `catch` \e -> x `seq` ...

then we can surely use

f x = IO $ \s -> x `seq` unIO (... `catch` ...)

I don't yet know enough about the demand analysis algorithm to see how to do this, but it seems rather likely to be worth trying.

comment:15 Changed 3 years ago by simonpj

Demand signatures are not expressive enough to express the either/or demand you want in comment:14. Sorry. Not even close.

My questions in comment:13 stand though. What goes wrong (in behavior or perf) if we just say catchException = catch?

comment:16 in reply to:  15 Changed 3 years ago by dfeuer

Replying to simonpj:

Demand signatures are not expressive enough to express the either/or demand you want in comment:14. Sorry. Not even close.

My questions in comment:13 stand though. What goes wrong (in behavior or perf) if we just say catchException = catch?

I haven't tried catchException = catch yet. I'll take a look at that soon. Are you sure demand signatures aren't close to expressive enough for what I want? Because they look fairly close. We have a way to talk about either-or demand for case branches. We have something fairly close to

m `catch#` f = \s -> case ORACLE m s of
  Succeeded -> m s
  Failed e -> f e s

If we're analyzing m catch# f, can't we find the demands for m applied to one argument and for f applied to two? Because then we can take their least upper bound and get demands for the whole thing applied to the realWorld#, right? That doesn't give catch# itself a terribly demand signature, but it shouldn't matter.

comment:17 Changed 3 years ago by simonpj

Yes, you could have a special rule for catch as we do for case I suppose. But we can't express it in the demand signature for catch; demand signatures aren't expressive enough.

But before cluttering up the demand analyser with special cases, I'd like to see evidence that it made a material difference. I doubt it would.

comment:18 Changed 3 years ago by dfeuer

Keywords: Exceptions added

comment:19 Changed 3 years ago by bgamari

Resolution: fixed
Status: newclosed

comment:20 Changed 18 months ago by simonpj

I'm looking at this exception-semantics stuff again, as I try to make sense of the nested CPR stuff #1600 and Phab:D4244.

I'm puzzled.

  • The strrictness analyser has quite a bit of complexity around ThrowsExn.
  • ThrowsExn only differs from Diverges if the ExnStr flag is set to ExnStr
  • And the only place that happens is in Demand.catchArgDmd
  • The only place catchArgDmd is used is in primops.txt.pp. Originally, in the patch for #11222, catchArgDmd was used for the first arg of catch#, catchSTM# and catchRetry#.
  • But the patch in this ticket removed two of those three; now only catchRetry# uses the catchArgDmd thing.

It loosk t me as if catchRetry# was left out by mistake. Right, David?

And if so, that's the last use of catchArgDmd and I think that all the tricky ThrowsExn machinery can disappear.

Incidentally, here's the amazing history of the strictness of catch#

  • Dec 13: 0558911f91c: catch# gets a strictness signature with lazyApply1Dmd
  • Jul 15: 7c0fff41789: catch# goes from lazyApply1Dmd to strictApply1Dmd
  • Dec 15: 28638dfe79e: catch# goes from strictApply1Dmd to lazyApply1Dmd. Trac #10712
  • Jan 16: 9915b656440: catch# goes from lazyApply1Dmd to catchArgDmd, and result goes from botRes to exnRes Trac #11222.
  • Mar 17: 701256df88c: catch# goes from 'catchArgDmd to lazyApply1Dmd`. This ticket #13330.

See also Note [Strictness for mask/unmask/catch] in primops.txt/pp.

comment:21 Changed 18 months ago by dfeuer

I thought you didn't like the performance implications and we reverted the change to TopDmd, but maybe I'm wrong. I can check if you like. I left the retry stuff alone on the basis that I didn't feel I understood it well enough.

comment:22 Changed 18 months ago by simonpj

OK thanks. I've opened #14998.

we reverted the change to TopDmd

I don't think so: the first arg of catch# is still lazyApply1Dmd. And maybe it should be.

Note: See TracTickets for help on using tickets.