#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by simonpj): * keywords: => Exceptions Old description:

Over time we have evolved into a messy and bad place for the interaction of strictness analysis and exceptions.

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`. * `exceptions_and_strictness` in `primops.txt/pp`. * `Note [Exceptions and strictness]` in `Demand.hs` * #11555, #13330, #10712, #11222

'''Item 1: ThrowsExn'''

* The strictness 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 looks to me as if `catchRetry#` was left out by mistake; and indeed David says "I left it out on the grounds that I didn't understand it well enough."

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

'''Item 2: strictness of catchException'''

As a result of all this to-and-fro we have in GHC.IO {{{ catch (IO io) handler = IO $ catch# io handler' catchException !io handler = catch io handler }}} That is, `catchException` is strict in the IO action itself. But Neil argues in #11555, commment:6 that this is bad because it breaks the monad laws.

I believe that there is some claimed performance gain from the strictness of `catchException`, but I don't believe it exists. The perf gain was from when it had a `strictApply1Dmd`; that is, it promised to call its argument. See this note in `primops.txt.pp` {{{ -- Note [Strictness for mask/unmask/catch] -- Consider this example, which comes from GHC.IO.Handle.Internals: -- wantReadableHandle3 f ma b st -- = case ... of -- DEFAULT -> case ma of MVar a -> ... -- 0# -> maskAsynchExceptions# (\st -> case ma of MVar a -> ...) -- The outer case just decides whether to mask exceptions, but we don't want -- thereby to hide the strictness in 'ma'! Hence the use of strictApply1Dmd. }}} I think the above saga was all in pursuit of exposing the strictness of `ma` if we used `catch#` instead of `maskAsynchExceptions#` in this example. But the saga ultimate appears to have failed, and the strictenss annotation in `catchException` is a vestige that carries no benefit, but does requires comments etc.

'''Item 3: performance'''

If making `catch#` have strictness `strictApply1Dmd` improves perf, it would be good to know where. That would entail making the (tiny) change, recompiling all, and nofibbing. Here is the commit message in 7c0fff41789, which added `strictApply1Dmd`: {{{ commit 7c0fff41789669450b02dc1db7f5d7babba5dee6 Author: Simon Peyton Jones Date: Tue Jul 21 12:28:42 2015 +0100

Improve strictness analysis for exceptions

Two things here:

* For exceptions-catching primops like catch#, we know that the main argument function will be called, so we can use strictApply1Dmd, rather than lazy

Changes in primops.txt.pp

* When a 'case' scrutinises a I/O-performing primop, the Note [IO hack in the demand analyser] was throwing away all strictness from the code that followed.

I found that this was causing quite a bit of unnecessary reboxing in the (heavily used) function GHC.IO.Handle.Internals.wantReadableHandle

So this patch prevents the hack applying when the case scrutinises a primop. See the revised Note [IO hack in the demand analyser]

Thse two things buy us quite a lot in programs that do a lot of IO.

Program Size Allocs Runtime Elapsed TotalMem -------------------------------------------------------------------------------- hpg -0.4% -2.9% -0.9% -1.0% +0.0% reverse-complem -0.4% -10.9% +10.7% +10.9% +0.0% simple -0.3% -0.0% +26.2% +26.2% +3.7% sphere -0.3% -6.3% 0.09 0.09 +0.0% -------------------------------------------------------------------------------- Min -0.7% -10.9% -4.6% -4.7% -1.7% Max -0.2% +0.0% +26.2% +26.2% +6.5% Geometric Mean -0.4% -0.3% +2.1% +2.1% +0.1% }}} If these gains are still on the table (i.e. moving to `strictApply1Dmd` gives them), perhaps we can add some strictness annotations to the I/O to replicate the effect, even if we'd decided we can't actualy make `catch#` strict?

'''Item 4: IO hack in the demand analyser'''

In getting up to speed with this, I noticed this comment in the demand analyser: {{{ {- Note [IO hack in the demand analyser] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ There's a hack here for I/O operations. Consider

case foo x s of { (# s', r #) -> y }

...omitted...

However, consider f x s = case getMaskingState# s of (# s, r #) -> case x of I# x2 -> ...

Here it is terribly sad to make 'f' lazy in 's'. After all, getMaskingState# is not going to diverge or throw an exception! This situation actually arises in GHC.IO.Handle.Internals.wantReadableHandle (on an MVar not an Int), and made a material difference.

So if the scrutinee is a primop call, we *don't* apply the state hack... }}} Idea: in the nested-CPR work, we have simple termination information. So we can use that, instead of "is a primop call" to deliver on this hack in a much more principled way.

New description: Over time we have evolved into a messy and bad place for the interaction of strictness analysis and exceptions. 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 * The [wiki:Exceptions] page * `Note [Strictness for mask/unmask/catch]` in `primops.txt/pp`. * `exceptions_and_strictness` in `primops.txt/pp`. * `Note [Exceptions and strictness]` in `Demand.hs` * #11555, #13330, #10712, #11222 '''Item 1: ThrowsExn''' * The strictness 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 looks to me as if `catchRetry#` was left out by mistake; and indeed David says "I left it out on the grounds that I didn't understand it well enough." And if so, that's the last use of `catchArgDmd` and I think that all the tricky `ThrowsExn` machinery can disappear. '''Item 2: strictness of catchException''' As a result of all this to-and-fro we have in GHC.IO {{{ catch (IO io) handler = IO $ catch# io handler' catchException !io handler = catch io handler }}} That is, `catchException` is strict in the IO action itself. But Neil argues in #11555, commment:6 that this is bad because it breaks the monad laws. I believe that there is some claimed performance gain from the strictness of `catchException`, but I don't believe it exists. The perf gain was from when it had a `strictApply1Dmd`; that is, it promised to call its argument. See this note in `primops.txt.pp` {{{ -- Note [Strictness for mask/unmask/catch] -- Consider this example, which comes from GHC.IO.Handle.Internals: -- wantReadableHandle3 f ma b st -- = case ... of -- DEFAULT -> case ma of MVar a -> ... -- 0# -> maskAsynchExceptions# (\st -> case ma of MVar a -> ...) -- The outer case just decides whether to mask exceptions, but we don't want -- thereby to hide the strictness in 'ma'! Hence the use of strictApply1Dmd. }}} I think the above saga was all in pursuit of exposing the strictness of `ma` if we used `catch#` instead of `maskAsynchExceptions#` in this example. But the saga ultimate appears to have failed, and the strictenss annotation in `catchException` is a vestige that carries no benefit, but does requires comments etc. '''Item 3: performance''' If making `catch#` have strictness `strictApply1Dmd` improves perf, it would be good to know where. That would entail making the (tiny) change, recompiling all, and nofibbing. Here is the commit message in 7c0fff41789, which added `strictApply1Dmd`: {{{ commit 7c0fff41789669450b02dc1db7f5d7babba5dee6 Author: Simon Peyton Jones Date: Tue Jul 21 12:28:42 2015 +0100 Improve strictness analysis for exceptions Two things here: * For exceptions-catching primops like catch#, we know that the main argument function will be called, so we can use strictApply1Dmd, rather than lazy Changes in primops.txt.pp * When a 'case' scrutinises a I/O-performing primop, the Note [IO hack in the demand analyser] was throwing away all strictness from the code that followed. I found that this was causing quite a bit of unnecessary reboxing in the (heavily used) function GHC.IO.Handle.Internals.wantReadableHandle So this patch prevents the hack applying when the case scrutinises a primop. See the revised Note [IO hack in the demand analyser] Thse two things buy us quite a lot in programs that do a lot of IO. Program Size Allocs Runtime Elapsed TotalMem -------------------------------------------------------------------------------- hpg -0.4% -2.9% -0.9% -1.0% +0.0% reverse-complem -0.4% -10.9% +10.7% +10.9% +0.0% simple -0.3% -0.0% +26.2% +26.2% +3.7% sphere -0.3% -6.3% 0.09 0.09 +0.0% -------------------------------------------------------------------------------- Min -0.7% -10.9% -4.6% -4.7% -1.7% Max -0.2% +0.0% +26.2% +26.2% +6.5% Geometric Mean -0.4% -0.3% +2.1% +2.1% +0.1% }}} If these gains are still on the table (i.e. moving to `strictApply1Dmd` gives them), perhaps we can add some strictness annotations to the I/O to replicate the effect, even if we'd decided we can't actualy make `catch#` strict? '''Item 4: IO hack in the demand analyser''' In getting up to speed with this, I noticed this comment in the demand analyser: {{{ {- Note [IO hack in the demand analyser] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ There's a hack here for I/O operations. Consider case foo x s of { (# s', r #) -> y } ...omitted... However, consider f x s = case getMaskingState# s of (# s, r #) -> case x of I# x2 -> ... Here it is terribly sad to make 'f' lazy in 's'. After all, getMaskingState# is not going to diverge or throw an exception! This situation actually arises in GHC.IO.Handle.Internals.wantReadableHandle (on an MVar not an Int), and made a material difference. So if the scrutinee is a primop call, we *don't* apply the state hack... }}} Idea: in the nested-CPR work, we have simple termination information. So we can use that, instead of "is a primop call" to deliver on this hack in a much more principled way. '''Item 5: raiseIO#''' There is an unresolved question about whether `raiseIO#` should return an exception result in its strictness signature. See Trac #13380, which applied a patch, then reverted it (on perf grounds) and then nothing. -- -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by sgraf): I have stolen and refined a [https://github.com/sgraf812/ghc-benchmark docker setup] from Joachim that rebuilds every diff from scratch in a deterministic environment, so I think I measured the right thing. I can re-generate the logs to see if something messed up, if you want. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by sgraf): * Attachment "nofib-table-all.txt" added. Addendum to comment:4: My docker setup was indeed messed up in that it didn't apply any diffs to the GHC checkout. I fixed that and this is the new nofib report. Although some numbers changed, this doesn't change the conclusion we made about item 3. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by sgraf): * Attachment "catchRetry.txt" added. NoFib results for item 1 -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): That's good. It'd be a big win. And we have no justification for treating `catchRetry#` specially. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by sgraf): `./validate` for the `catchRetry#` changes (item 1) found no issues. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by sgraf): This is already reproducible for hello world. For the following program: {{{#!hs main = mapM_ (\_ -> putStrLn "hi") [0..10000] }}} we have this `+RTS -s` output under `-O2` (first HEAD, second with bangs removed in GHC.IO): {{{ 9,491,968 bytes allocated in the heap 15,504 bytes copied during GC 44,576 bytes maximum residency (2 sample(s)) 29,152 bytes maximum slop 0 MB total memory in use (0 MB lost due to fragmentation) Tot time (elapsed) Avg pause Max pause Gen 0 8 colls, 0 par 0.000s 0.000s 0.0000s 0.0000s Gen 1 2 colls, 0 par 0.000s 0.000s 0.0002s 0.0002s INIT time 0.000s ( 0.000s elapsed) MUT time 0.005s ( 0.005s elapsed) GC time 0.000s ( 0.000s elapsed) EXIT time 0.000s ( 0.000s elapsed) Total time 0.006s ( 0.006s elapsed) %GC time 0.0% (0.0% elapsed) Alloc rate 1,932,804,477 bytes per MUT second Productivity 89.2% of total user, 89.5% of total elapsed }}} {{{ 10,132,096 bytes allocated in the heap 16,360 bytes copied during GC 44,576 bytes maximum residency (2 sample(s)) 29,152 bytes maximum slop 0 MB total memory in use (0 MB lost due to fragmentation) Tot time (elapsed) Avg pause Max pause Gen 0 8 colls, 0 par 0.000s 0.000s 0.0000s 0.0000s Gen 1 2 colls, 0 par 0.000s 0.000s 0.0002s 0.0003s INIT time 0.000s ( 0.000s elapsed) MUT time 0.007s ( 0.007s elapsed) GC time 0.001s ( 0.001s elapsed) EXIT time 0.000s ( 0.000s elapsed) Total time 0.007s ( 0.007s elapsed) %GC time 0.0% (0.0% elapsed) Alloc rate 1,516,022,826 bytes per MUT second Productivity 89.6% of total user, 89.8% of total elapsed }}} So, the working set is affected, too, which I think is a necessary condition for a space leak. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:14 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by sgraf): Ah, `-ddump-prep` revealed that the non-trivial argument to `catch#` on non-HEAD was turned into a one-shot PAP `sat = act h`, which allocates. Of course! That's probably where the alleged benefit of item 3 came from. Indeed `reverse-complement` and `spheres` where the biggest beneficiaries, judging from the commit message. So, we need to be strict in `act h` because otherwise it will turn into a lazy let that allocates. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:16 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by sgraf): Also shouldn't `catch#` at least be head-strict in its first argument? I'm pretty sure that would get rid of the bang pattern I had to introduce, while still having correct semantics: An `IO` action which throws ''when executed'' triggers the exception handler, while a call like `catch undefined h == undefined`, something I would expect. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:18 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by sgraf): * cc: dfeuer (added) Comment: CCing @dfeuer, as he seems to have ample experience with `catch#`. No need to react right now, just let me know if you see me going anywhere you already were. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:20 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by sgraf): Re: Making `want*Handle` strict in their `act`ions in comment:19. This regresses all over the place and I don't really know why. Maybe because `case f x of g -> case g y of res -> res` is operationally different than `case f x y of res -> res`? Maybe it's because one-shot information worsened? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:22 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by Ben Gamari ): In [changeset:"00b8ecb78624511a045120673b01fafe5794ecdc/ghc" 00b8ecb7/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="00b8ecb78624511a045120673b01fafe5794ecdc" Declare `catchRetry#` lazy in its first argument As per the results on item 1 in T14998, declaring `catchRetry#` lazy in its first argument opens the possibility to remove `ExnStr` complexity from strictness demands at virtually no regressions in NoFib. This brings `catchRetry#` in line with other primops from the `catch*` family. Reviewers: bgamari, simonpj, nomeata Reviewed By: bgamari Subscribers: thomie, carter GHC Trac Issues: #14998, #11222 Differential Revision: https://phabricator.haskell.org/D4573 }}} -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:24 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by sgraf): Replying to [comment:25 simonpj]:

...

Declare `catchRetry#` lazy in its first argument

Great. So now we can dump all the `ThrowsExn` machinery! sgraf, are you up for doing that?

I've added it to my queue. I can probably give it a higher priority, given that it's mostly syntactic and (hopefully) low effort. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:26 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by dfeuer): I'd be happy to give that a crack if you like! -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:28 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by sgraf): Just to sum up this thread so far: - Item 1 (make `catchRetry#` lazy) was merged and @dfeuer works on removing `ExnStr` from `Demand.hs` on `wip/no-exnstr`. - Item 2 was about finding out where things regress if we got rid of the strictness introduced in `catchException` and `catchAny`. I summarised my debugging sessions in [comment:16]. - Item 3 (make `catch#` strict and see if it improves things) was handled in [comment:2]. Contrary to prior measurements, I couldn't make out huge improvements, probably due to [comment:17]. I didn't bother to pinpoint where the -0.1% came from, though. - Item 4 (IO hack) isn't fully resolved yet, but I addressed it in [comment:21]. @dfeuer (probably?) wrote down his thoughts on this on [wiki:Exceptions/PreciseExceptions#Implementingpreciseexceptions]. - Item 5 (strictness of `raiseIO#`) is still open. I don't think I can address this at all, lacking too much background. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:30 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: #11555 #13330 | Differential Rev(s): #10712 #11222 #13380 | Wiki Page: | -------------------------------------+------------------------------------- Comment (by dfeuer): I realized something. The `ExnStr` + `ThrowsExn` idea is the wrong idea ''for its purpose'', but it might be the right idea for a ''different'' purpose. Its goal (as I understand it) was to let us say that {{{#!hs m `catch` h }}} is sort of strict in `m`, and therefore we can evaluate `m` somewhat eagerly. This didn't work out, because we don't have enough information to do it right. However, what it ''should'' let us do correctly is a certain amount of dead code elimination. In particular, if we see that `m` definitely ''diverges'' (without throwing an exception), then we can simplify `catch m h` to `m`. But I'm betting this benefit isn't worth the complexity. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:32 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

I realized something. The `ExnStr` + `ThrowsExn` idea is the wrong idea ''for its purpose'', but it might be the right idea for a ''different''

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions, | DemandAnalysis Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: #11555 #13330 | Differential Rev(s): #10712 #11222 #13380 | Wiki Page: | -------------------------------------+------------------------------------- Comment (by sgraf): Replying to [comment:32 dfeuer]: purpose. Its goal (as I understand it) was to let us say that

{{{#!hs m `catch` h }}}

is sort of strict in `m`, and therefore we can evaluate `m` somewhat

eagerly. This didn't work out, because we don't have enough information to do it right. However, what it ''should'' let us do correctly is a certain amount of dead code elimination. In particular, if we see that `m` definitely ''diverges'' (without throwing an exception), then we can simplify `catch m h` to `m`. But I'm betting this benefit isn't worth the complexity. I'm currently messing with Demand Analysis again and ExnStr has become a huge pain. I'll prepare a patch to remove it. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:34 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#14998: Sort out the strictness mess for exceptions -------------------------------------+------------------------------------- Reporter: simonpj | Owner: (none) Type: bug | Status: new Priority: normal | Milestone: 8.4.3 Component: Compiler | Version: 8.2.2 Resolution: | Keywords: Exceptions, | DemandAnalysis Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: #11555 #13330 | Differential Rev(s): #10712 #11222 #13380 | Wiki Page: | -------------------------------------+------------------------------------- Description changed by simonpj: Old description:

Over time we have evolved into a messy and bad place for the interaction of strictness analysis and exceptions.

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 * The [wiki:Exceptions] page * `Note [Strictness for mask/unmask/catch]` in `primops.txt/pp`. * `exceptions_and_strictness` in `primops.txt/pp`. * `Note [Exceptions and strictness]` in `Demand.hs` * #11555, #13330, #10712, #11222

'''Item 1: ThrowsExn'''

* The strictness 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 looks to me as if `catchRetry#` was left out by mistake; and indeed David says "I left it out on the grounds that I didn't understand it well enough."

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

'''Item 2: strictness of catchException'''

As a result of all this to-and-fro we have in GHC.IO {{{ catch (IO io) handler = IO $ catch# io handler' catchException !io handler = catch io handler }}} That is, `catchException` is strict in the IO action itself. But Neil argues in #11555, commment:6 that this is bad because it breaks the monad laws.

I believe that there is some claimed performance gain from the strictness of `catchException`, but I don't believe it exists. The perf gain was from when it had a `strictApply1Dmd`; that is, it promised to call its argument. See this note in `primops.txt.pp` {{{ -- Note [Strictness for mask/unmask/catch] -- Consider this example, which comes from GHC.IO.Handle.Internals: -- wantReadableHandle3 f ma b st -- = case ... of -- DEFAULT -> case ma of MVar a -> ... -- 0# -> maskAsynchExceptions# (\st -> case ma of MVar a -> ...) -- The outer case just decides whether to mask exceptions, but we don't want -- thereby to hide the strictness in 'ma'! Hence the use of strictApply1Dmd. }}} I think the above saga was all in pursuit of exposing the strictness of `ma` if we used `catch#` instead of `maskAsynchExceptions#` in this example. But the saga ultimate appears to have failed, and the strictenss annotation in `catchException` is a vestige that carries no benefit, but does requires comments etc.

'''Item 3: performance'''

If making `catch#` have strictness `strictApply1Dmd` improves perf, it would be good to know where. That would entail making the (tiny) change, recompiling all, and nofibbing. Here is the commit message in 7c0fff41789, which added `strictApply1Dmd`: {{{ commit 7c0fff41789669450b02dc1db7f5d7babba5dee6 Author: Simon Peyton Jones Date: Tue Jul 21 12:28:42 2015 +0100

Improve strictness analysis for exceptions

Two things here:

* For exceptions-catching primops like catch#, we know that the main argument function will be called, so we can use strictApply1Dmd, rather than lazy

Changes in primops.txt.pp

* When a 'case' scrutinises a I/O-performing primop, the Note [IO hack in the demand analyser] was throwing away all strictness from the code that followed.

I found that this was causing quite a bit of unnecessary reboxing in the (heavily used) function GHC.IO.Handle.Internals.wantReadableHandle

So this patch prevents the hack applying when the case scrutinises a primop. See the revised Note [IO hack in the demand analyser]

Thse two things buy us quite a lot in programs that do a lot of IO.

Program Size Allocs Runtime Elapsed TotalMem -------------------------------------------------------------------------------- hpg -0.4% -2.9% -0.9% -1.0% +0.0% reverse-complem -0.4% -10.9% +10.7% +10.9% +0.0% simple -0.3% -0.0% +26.2% +26.2% +3.7% sphere -0.3% -6.3% 0.09 0.09 +0.0% -------------------------------------------------------------------------------- Min -0.7% -10.9% -4.6% -4.7% -1.7% Max -0.2% +0.0% +26.2% +26.2% +6.5% Geometric Mean -0.4% -0.3% +2.1% +2.1% +0.1% }}} If these gains are still on the table (i.e. moving to `strictApply1Dmd` gives them), perhaps we can add some strictness annotations to the I/O to replicate the effect, even if we'd decided we can't actualy make `catch#` strict?

'''Item 4: IO hack in the demand analyser'''

In getting up to speed with this, I noticed this comment in the demand analyser: {{{ {- Note [IO hack in the demand analyser] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ There's a hack here for I/O operations. Consider

case foo x s of { (# s', r #) -> y }

...omitted...

However, consider f x s = case getMaskingState# s of (# s, r #) -> case x of I# x2 -> ...

Here it is terribly sad to make 'f' lazy in 's'. After all, getMaskingState# is not going to diverge or throw an exception! This situation actually arises in GHC.IO.Handle.Internals.wantReadableHandle (on an MVar not an Int), and made a material difference.

So if the scrutinee is a primop call, we *don't* apply the state hack... }}} Idea: in the nested-CPR work, we have simple termination information. So we can use that, instead of "is a primop call" to deliver on this hack in a much more principled way.

'''Item 5: raiseIO#'''

There is an unresolved question about whether `raiseIO#` should return an exception result in its strictness signature. See Trac #13380, which applied a patch, then reverted it (on perf grounds) and then nothing.

New description: Over time we have evolved into a messy and bad place for the interaction of strictness analysis and exceptions. 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 * The [wiki:Exceptions] page * `Note [Strictness for mask/unmask/catch]` in `primops.txt/pp`. * `exceptions_and_strictness` in `primops.txt/pp`. * `Note [Exceptions and strictness]` in `Demand.hs` * #11555, #13330, #10712, #11222 '''Item 1: ThrowsExn''' * The strictness 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 looks to me as if `catchRetry#` was left out by mistake; and indeed David says "I left it out on the grounds that I didn't understand it well enough." And if so, that's the last use of `catchArgDmd` and I think that all the tricky `ThrowsExn` machinery can disappear. '''Item 2: strictness of catchException''' As a result of all this to-and-fro we have in GHC.IO {{{ catch (IO io) handler = IO $ catch# io handler' catchException !io handler = catch io handler }}} That is, `catchException` is strict in the IO action itself. But Neil argues in #11555, commment:6 that this is bad because it breaks the monad laws. I believe that there is some claimed performance gain from the strictness of `catchException`, but I don't believe it exists. The perf gain was from when it had a `strictApply1Dmd`; that is, it promised to call its argument. See this note in `primops.txt.pp` {{{ -- Note [Strictness for mask/unmask/catch] -- Consider this example, which comes from GHC.IO.Handle.Internals: -- wantReadableHandle3 f ma b st -- = case ... of -- DEFAULT -> case ma of MVar a -> ... -- 0# -> maskAsynchExceptions# (\st -> case ma of MVar a -> ...) -- The outer case just decides whether to mask exceptions, but we don't want -- thereby to hide the strictness in 'ma'! Hence the use of strictApply1Dmd. }}} I think the above saga was all in pursuit of exposing the strictness of `ma` if we used `catch#` instead of `maskAsynchExceptions#` in this example. But the saga ultimate appears to have failed, and the strictenss annotation in `catchException` is a vestige that carries no benefit, but does requires comments etc. '''Item 3: performance''' If making `catch#` have strictness `strictApply1Dmd` improves perf, it would be good to know where. That would entail making the (tiny) change, recompiling all, and nofibbing. Here is the commit message in 7c0fff41789, which added `strictApply1Dmd`: {{{ commit 7c0fff41789669450b02dc1db7f5d7babba5dee6 Author: Simon Peyton Jones Date: Tue Jul 21 12:28:42 2015 +0100 Improve strictness analysis for exceptions Two things here: * For exceptions-catching primops like catch#, we know that the main argument function will be called, so we can use strictApply1Dmd, rather than lazy Changes in primops.txt.pp * When a 'case' scrutinises a I/O-performing primop, the Note [IO hack in the demand analyser] was throwing away all strictness from the code that followed. I found that this was causing quite a bit of unnecessary reboxing in the (heavily used) function GHC.IO.Handle.Internals.wantReadableHandle So this patch prevents the hack applying when the case scrutinises a primop. See the revised Note [IO hack in the demand analyser] Thse two things buy us quite a lot in programs that do a lot of IO. Program Size Allocs Runtime Elapsed TotalMem -------------------------------------------------------------------------------- hpg -0.4% -2.9% -0.9% -1.0% +0.0% reverse-complem -0.4% -10.9% +10.7% +10.9% +0.0% simple -0.3% -0.0% +26.2% +26.2% +3.7% sphere -0.3% -6.3% 0.09 0.09 +0.0% -------------------------------------------------------------------------------- Min -0.7% -10.9% -4.6% -4.7% -1.7% Max -0.2% +0.0% +26.2% +26.2% +6.5% Geometric Mean -0.4% -0.3% +2.1% +2.1% +0.1% }}} If these gains are still on the table (i.e. moving to `strictApply1Dmd` gives them), perhaps we can add some strictness annotations to the I/O to replicate the effect, even if we'd decided we can't actualy make `catch#` strict? '''Item 4: IO hack in the demand analyser''' In getting up to speed with this, I noticed this comment in the demand analyser: {{{ {- Note [IO hack in the demand analyser] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ There's a hack here for I/O operations. Consider case foo x s of { (# s', r #) -> y } ...omitted... However, consider f x s = case getMaskingState# s of (# s, r #) -> case x of I# x2 -> ... Here it is terribly sad to make 'f' lazy in 's'. After all, getMaskingState# is not going to diverge or throw an exception! This situation actually arises in GHC.IO.Handle.Internals.wantReadableHandle (on an MVar not an Int), and made a material difference. So if the scrutinee is a primop call, we *don't* apply the state hack... }}} Idea: in the nested-CPR work, we have simple termination information. So we can use that, instead of "is a primop call" to deliver on this hack in a much more principled way. '''Item 5: raiseIO#''' There is an unresolved question about whether `raiseIO#` should return an exception result in its strictness signature. See Trac #13380, which applied a patch, then reverted it (on perf grounds) and then nothing. -- -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/14998#comment:36 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler