Opened 18 months ago

Last modified 8 months ago

#14998 new bug

Sort out the strictness mess for exceptions

Reported by: simonpj Owned by:
Priority: normal Milestone: 8.4.3
Component: Compiler Version: 8.2.2
Keywords: Exceptions, DemandAnalysis Cc: dfeuer, sgraf
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: #11555 #13330 #10712 #11222 #13380 Differential Rev(s):
Wiki Page:

Description (last modified by simonpj)

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 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 <simonpj@microsoft.com>
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.

Attachments (4)

nofib-table-all.txt (195.8 KB) - added by sgraf 18 months ago.
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.
catchException.txt (195.8 KB) - added by sgraf 18 months ago.
Results for Item 2
catchRetry.txt (195.8 KB) - added by sgraf 18 months ago.
NoFib results for item 1
catchException.2.txt (195.8 KB) - added by sgraf 18 months ago.
Results for Item 2 after fixing do_operation

Download all attachments as: .zip

Change History (40)

comment:1 Changed 18 months ago by simonpj

Description: modified (diff)
Keywords: Exceptions added

comment:2 Changed 18 months ago by sgraf

This measurement is for item 3 above for the following diff:

diff --git a/compiler/prelude/primops.txt.pp b/compiler/prelude/primops.txt.pp
index 43e8f535d3..c80b99268c 100644
--- a/compiler/prelude/primops.txt.pp
+++ b/compiler/prelude/primops.txt.pp
@@ -2051,7 +2051,7 @@ primop  CatchOp "catch#" GenPrimOp
        -> State# RealWorld
        -> (# State# RealWorld, a #)
    with
-   strictness  = { \ _arity -> mkClosedStrictSig [ lazyApply1Dmd
+   strictness  = { \ _arity -> mkClosedStrictSig [ strictApply1Dmd
                                                  , lazyApply2Dmd
                                                  , topDmd] topRes }
                  -- See Note [Strictness for mask/unmask/catch]

More or less no change, except for -0.1% in counted instructions for a few benchmarks.

Last edited 18 months ago by sgraf (previous) (diff)

comment:3 Changed 18 months ago by simonpj

Interesting! That includes recompiling all the libraries? (There are no explicit catch calls in nofib.)

comment:4 Changed 18 months ago by sgraf

I have stolen and refined a 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.

comment:5 Changed 18 months ago by simonpj

OK, well, let's declare victory on Item 4. Maybe it was the mask# primops (which still have strictArgDmd that were delivering the payoff.

Next up: item 2. Could you remove the ! from the defns of GHC.IO.catchException and catchAny, and see if that has any perf impact? (And validate.)

I think they are unnecessary.

Next: item 1. Could you make catchRetry# have lazyApply1Dmd like catch# and see if that has any effect? If not, let's switch it.

Thanks

Changed 18 months ago 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.

Changed 18 months ago by sgraf

Attachment: catchException.txt added

Results for Item 2

comment:6 Changed 18 months ago by sgraf

Re Item 2: A net loss (0.2% Allocs, 0.1% Instr). reverse-complement allocates 11.6% more, sphere executes 4.4% more instructions, binary-trees 0.9% less.

This was the diff:

diff --git a/libraries/base/GHC/IO.hs b/libraries/base/GHC/IO.hs
index 118ebea..554b4e3 100644
--- a/libraries/base/GHC/IO.hs
+++ b/libraries/base/GHC/IO.hs
@@ -142,7 +142,7 @@ have to work around that in the definition of catch below).
 -- @catchException undefined b == _|_@. See #exceptions_and_strictness#
 -- for details.
 catchException :: Exception e => IO a -> (e -> IO a) -> IO a
-catchException !io handler = catch io handler
+catchException io handler = catch io handler
 
 -- | This is the simplest of the exception-catching functions.  It
 -- takes a single argument, runs it, and if an exception is raised
@@ -194,7 +194,7 @@ catch (IO io) handler = IO $ catch# io handler'
 -- @catchAny undefined b == _|_@. See #exceptions_and_strictness# for
 -- details.
 catchAny :: IO a -> (forall e . Exception e => e -> IO a) -> IO a
-catchAny !(IO io) handler = IO $ catch# io handler'
+catchAny (IO io) handler = IO $ catch# io handler'
     where handler' (SomeException e) = unIO (handler e)
 
 -- Using catchException here means that if `m` throws an

comment:7 Changed 18 months ago by simonpj

reverse-complement allocates 11.6% more

That's remarkable. I expected it to be virtually nothing. I suppose that some investigation is called for.

Changed 18 months ago by sgraf

Attachment: catchRetry.txt added

NoFib results for item 1

comment:8 Changed 18 months ago by sgraf

The diff I used for item 1:

diff --git a/compiler/prelude/primops.txt.pp b/compiler/prelude/primops.txt.pp
index 1362704074..4c65ba4220 100644
--- a/compiler/prelude/primops.txt.pp
+++ b/compiler/prelude/primops.txt.pp
@@ -2377,7 +2377,7 @@ primop  CatchRetryOp "catchRetry#" GenPrimOp
    -> (State# RealWorld -> (# State# RealWorld, a #) )
    -> (State# RealWorld -> (# State# RealWorld, a #) )
    with
-   strictness  = { \ _arity -> mkClosedStrictSig [ catchArgDmd
+   strictness  = { \ _arity -> mkClosedStrictSig [ lazyApply1Dmd
                                                  , lazyApply1Dmd
                                                  , topDmd ] topRes }
                  -- See Note [Strictness for mask/unmask/catch]

No change in allocations, max +0.1% in counted instructions for scc. I have yet to run a validate, but this seems worthwhile if it would get rid of ExnStr!

comment:9 Changed 18 months ago by simonpj

That's good. It'd be a big win. And we have no justification for treating catchRetry# specially.

comment:10 in reply to:  7 Changed 18 months ago by sgraf

Replying to simonpj:

reverse-complement allocates 11.6% more

That's remarkable. I expected it to be virtually nothing. I suppose that some investigation is called for.

Long story short, there was no difference in generated Core for both reverse-complement and sphere.

The difference is is due to the do_operation function that has a call to catchException. When I compare the strictness signatures when compiling GHC.IO.Handle.Internal there is a notable difference for the withHandle* family of functions which calls do_operation:

36,38c36,38
< GHC.IO.Handle.Internals.withAllHandles__: <L,U><S,1*U><L,C(C1(U))><S,U>
< GHC.IO.Handle.Internals.withHandle: <L,U><S,1*U><L,C(C1(U))><S,U>
< GHC.IO.Handle.Internals.withHandle': <L,U><L,U><S(S),U(U)><L,C(C1(U))><S,U>
---
> GHC.IO.Handle.Internals.withAllHandles__: <L,U><S,1*U><C(S),C(U)><S,U>
> GHC.IO.Handle.Internals.withHandle: <L,U><S,1*U><C(S),C(U)><S,U>
> GHC.IO.Handle.Internals.withHandle': <L,U><L,U><S(S),U(U)><C(S),C(U)><S,U>
41c41
< GHC.IO.Handle.Internals.withHandle__': <L,U><L,U><S(S),U(U)><L,C(C1(U))><S,U>
---
> GHC.IO.Handle.Internals.withHandle__': <L,U><L,U><S(S),U(U)><C(S),C(U)><S,U>
80,82c80,82
< GHC.IO.Handle.Internals.withAllHandles__: <L,U><S,1*U><L,C(C1(U))><S,U>
< GHC.IO.Handle.Internals.withHandle: <L,U><S,1*U><L,C(C1(U))><S,U>
< GHC.IO.Handle.Internals.withHandle': <L,U><L,U><S(S),1*U(U)><L,C(C1(U))><S,U>
---
> GHC.IO.Handle.Internals.withAllHandles__: <L,U><S,1*U><C(S),C(U)><S,U>
> GHC.IO.Handle.Internals.withHandle: <L,U><S,1*U><C(S),C(U)><S,U>
> GHC.IO.Handle.Internals.withHandle': <L,U><L,U><S(S),1*U(U)><C(S),C(U)><S,U>
85c85
< GHC.IO.Handle.Internals.withHandle__': <L,U><L,U><S(S),1*U(U)><L,C(C1(U))><S,U>
---
> GHC.IO.Handle.Internals.withHandle__': <L,U><L,U><S(S),1*U(U)><C(S),C(U)><S,U>

These are called transitively by hGetBuf and hPutBuf. We could annotate the IO action passed to do_operation for an easy fix:

diff --git a/libraries/base/GHC/IO/Handle/Internals.hs b/libraries/base/GHC/IO/Handle/Internals.hs
index 48ece1dc5e..3d58f3d3bc 100644
--- a/libraries/base/GHC/IO/Handle/Internals.hs
+++ b/libraries/base/GHC/IO/Handle/Internals.hs
@@ -163,7 +163,8 @@ do_operation :: String -> Handle -> (Handle__ -> IO a) -> MVar Handle__ -> IO a
 do_operation fun h act m = do
   h_ <- takeMVar m
   checkHandleInvariants h_
-  act h_ `catchException` handler h_
+  let !io = act h_
+  io `catchException` handler h_
   where
     handler h_ e = do
       putMVar m h_

I'm not sure if that's just navigating around the problem, but this brings strictness signatures on par and fixes sphere and reverse-complement. I'll re-run benchmarks now. Also, without this patch, T12996 (#12996) broke in ./validate.

comment:11 Changed 18 months ago by sgraf

./validate for the catchRetry# changes (item 1) found no issues.

Changed 18 months ago by sgraf

Attachment: catchException.2.txt added

Results for Item 2 after fixing do_operation

comment:12 Changed 18 months ago by sgraf

Item 2 no longer regresses in performance and passes ./validate with this diff:

diff --git a/libraries/base/GHC/IO.hs b/libraries/base/GHC/IO.hs
index 05ad277127..f9dbfef71b 100644
--- a/libraries/base/GHC/IO.hs
+++ b/libraries/base/GHC/IO.hs
@@ -142,7 +142,7 @@ have to work around that in the definition of catch below).
 -- @catchException undefined b == _|_@. See #exceptions_and_strictness#
 -- for details.
 catchException :: Exception e => IO a -> (e -> IO a) -> IO a
-catchException !io handler = catch io handler
+catchException io handler = catch io handler
 
 -- | This is the simplest of the exception-catching functions.  It
 -- takes a single argument, runs it, and if an exception is raised
@@ -194,7 +194,7 @@ catch (IO io) handler = IO $ catch# io handler'
 -- @catchAny undefined b == _|_@. See #exceptions_and_strictness# for
 -- details.
 catchAny :: IO a -> (forall e . Exception e => e -> IO a) -> IO a
-catchAny !(IO io) handler = IO $ catch# io handler'
+catchAny (IO io) handler = IO $ catch# io handler'
     where handler' (SomeException e) = unIO (handler e)
 
 -- Using catchException here means that if `m` throws an
diff --git a/libraries/base/GHC/IO/Handle/Internals.hs b/libraries/base/GHC/IO/Handle/Internals.hs
index 48ece1dc5e..3d58f3d3bc 100644
--- a/libraries/base/GHC/IO/Handle/Internals.hs
+++ b/libraries/base/GHC/IO/Handle/Internals.hs
@@ -163,7 +163,8 @@ do_operation :: String -> Handle -> (Handle__ -> IO a) -> MVar Handle__ -> IO a
 do_operation fun h act m = do
   h_ <- takeMVar m
   checkHandleInvariants h_
-  act h_ `catchException` handler h_
+  let !io = act h_
+  io `catchException` handler h_
   where
     handler h_ e = do
       putMVar m h_

comment:13 Changed 18 months ago by sgraf

... Although it's unclear to me why the bang has such a dramatic effect.

I presumed that the call to hGetBuf and hPutBuf were responsible for this, but going up from do_operation, I can't see any changes in Core resulting from the changed strictness signatures. In fact, the activations go like this:

   GHC.IO.Handle.Text.hPutBuf
-> GHC.IO.Handle.Internals.wantWritableHandle
-> GHC.IO.Handle.Internals.withHandle_'
-> GHC.IO.Handle.Internals.withHandle'

Note from the first diff in comment:10, that while the signature for withHandle' differs relative to HEAD, that is not the case for withHandle_' (hooray for names)! withHandle_' is lazy in its action parameter in both versions. It should probably have C(S), too, if it has in withHandle', but that's a separate issue.

Since there is no difference in signatures for withHandle_', there is no flow in strictness information to callees, which means that the bang just fixes some kind of space leak. Which is weird, because I don't see any big data structures involved with forcing the result of the action.

comment:14 Changed 18 months ago by sgraf

This is already reproducible for hello world. For the following program:

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.

Edit: I measure an increase of 64 bytes allocated per loop iteration.

Last edited 18 months ago by sgraf (previous) (diff)

comment:15 Changed 18 months ago by sgraf

There's no binary difference in dump-cmm for the hello world example (as expected) and there is no binary difference in the object files generated for GHC.IO.Handle.Text either, after stripping.

The only difference in GHC.IO.Handle.Internals seems to be in do_operation (no wonder, that was the only place Core differed). Here's the diff. You can see that HEAD on the right somehow can jump to cbPP directly, whereas the patched version on the left needs to allocate a closure for actually calling the action (which I presume got ripped out into the sat_entry at the top). I'm not proficient enough with Stg/Cmm to tell why the single bang leads to better code, while the diff in Core is boring. Maybe a suprise hides in the way catch# is lowered?

So, what should a Note look like that explains Phab:D4574? Just 'plugging a space leak'?

comment:16 Changed 18 months ago 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.

comment:17 Changed 18 months ago by sgraf

Also note how the improvements for changeset:9915b6564403a6d17651e9969e9ea5d7d7e78e7f (from 2016, from #11222) match those of changeset:7c0fff41789669450b02dc1db7f5d7babba5dee6 (from 2015, subject of item 3).

Strictness with catch# is now a little subtle (if also a lot more correct), because callers have to make sure that their action is forced before calling it. I wonder if we get more improvements if we make withHandle_' and friends strict in their action parameter...

comment:18 Changed 18 months ago 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.

Edit:

Ok, weird

> catch (error "this shouldn't be caught") $ \(_ :: SomeException) -> putStrLn "and this should be unreachable"
and this shouldn't be reachable

This is not what I had expected... Well, then just ignore this comment.

Last edited 18 months ago by sgraf (previous) (diff)

comment:19 Changed 18 months ago by sgraf

As expressed in Phab:D4574#126789 I have doubts that we should really merge the diff for item 2. According to #exceptions_and_strictness# in GHC.IO catchException and catchAny have that bang on purpose, as a helper function when catch# moved to a lazy signature in #11555. ticket:11222#comment:6 is very relevant. Such a raiseIO# is exactly what catchException and catchAny would need. I misunderstood that comment.

Somewhere above, I discovered that GHC.IO.Handle.Internals.want*Handle aren't strict in their actions. I'm currently evaluating any performance benefits by adding bangs in appropriate positions.

Last edited 18 months ago by sgraf (previous) (diff)

comment:20 Changed 18 months ago by sgraf

Cc: dfeuer added

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.

comment:21 Changed 18 months ago by sgraf

Re: item 4. Isn't exprOkForSpeculation or exprOkForSideEffects exactly what we want, rather than just isPrimOpId? I don't think the limited convergence checking that nested CPR would bring (it should not allow arbitrary costly converging expression anyway) has any benefit over those functions. I could give those a try, if you want.

comment:22 Changed 18 months ago by sgraf

Re: Making want*Handle strict in their actions 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?

comment:23 Changed 18 months ago by dfeuer

Ah, I knew something around here got reverted on performance grounds. Sorry I mixed up which bit.

comment:24 Changed 17 months ago by Ben Gamari <ben@…>

In 00b8ecb7/ghc:

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

comment:25 Changed 17 months ago by 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?

comment:26 in reply to:  25 Changed 17 months ago by sgraf

Replying to 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.

comment:27 Changed 17 months ago by sgraf

Cc: sgraf added

comment:28 Changed 17 months ago by dfeuer

I'd be happy to give that a crack if you like!

comment:29 in reply to:  28 Changed 17 months ago by sgraf

Replying to dfeuer:

I'd be happy to give that a crack if you like!

Sure then, it's all yours!

comment:30 Changed 17 months ago 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 16.
  • Item 3 (make catch# strict and see if it improves things) was handled in 2. Contrary to prior measurements, I couldn't make out huge improvements, probably due to 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 21. @dfeuer (probably?) wrote down his thoughts on this on Exceptions/PreciseExceptions.
  • Item 5 (strictness of raiseIO#) is still open. I don't think I can address this at all, lacking too much background. See FixingExceptions.
Last edited 17 months ago by sgraf (previous) (diff)

comment:31 Changed 17 months ago by sgraf

comment:32 Changed 17 months ago 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

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.

comment:33 Changed 8 months ago by simonpj

Keywords: DemandAnalysis added

comment:34 in reply to:  32 Changed 8 months ago by sgraf

Replying to 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

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.

Edit: And I don't see how dead code concerns should affect strictness analysis. That 'if m diverges, then catch m h => m' transformation is something the simplifier could do, though.

Last edited 8 months ago by sgraf (previous) (diff)

comment:35 Changed 8 months ago by simonpj

I'm currently messing with Demand Analysis again and ExnStr has become a huge pain. I'll prepare a patch to remove it.

Hurrah. That's Item 1 from the Description.

comment:36 Changed 8 months ago by simonpj

Description: modified (diff)
Note: See TracTickets for help on using tickets.