Fix demand analysis for catchSTM# and catchRetry#
AbandonedPublic

Authored by dfeuer on Mar 2 2017, 4:52 PM.

Details

Reviewers
austin
bgamari
Trac Issues
#13357
Summary
  • catchSTM# appears to have the same strictness behavior as

catch#, so we conservatively give it a lazy demand signature.
catchRetry# appears to be much stricter in its first argument,
so we give it a strict demand signature.

  • Remove all the defective machinery for catch arguments from

Demand.hs. If we want, we can try to write something a bit
clever some day that's actually correct. I think its structure
will likely be a good bit different. To a probably-close-enough
approximation,

catch# m f s = case ORACLE m s of
  Succeeds -> m s
  Fails ex -> f ex s

so we could use case-like demand analysis for catch# and
catchSTM. It remains an open question whether this would
actually reveal enough strictness to be worth the trouble.

Fixes Trac #13357

dfeuer created this revision.Mar 2 2017, 4:52 PM
dfeuer edited the summary of this revision. (Show Details)Mar 2 2017, 4:53 PM
dfeuer updated the Trac tickets for this revision.
bgamari requested changes to this revision.Mar 2 2017, 6:32 PM

Other than style issues the implementation itself looks fine. However, the documentation needs elaboration. The commit message should describe what in particular was wrong with the previous approach. The note should make a brief mention of what was tried previously and why it didn't work. There has been a long road to get here but currently neither the commit message nor the Note do it justice. Be sure to mention 9915b6564403a6d17651e9969e9ea5d7d7e78e7f and the relevant tickets.

Also, it's fine to describe a proposed alternative demand scheme in the commit message, but it needs adequate description; I have no idea what ORACLE is.

In general it is far too easy to forget these details, especially when there aren't clear references to the relevant tickets.

compiler/basicTypes/Demand.hs
133–157

Surely we have learned something from the previous attempt at cleverness. We should document it here along with a reference to Trac #11222. What did we try? Why didn't it work?

Also, this really ought to reference Note [Strictness for mask/unmask/catch].

208

Maintain alignment

218

Alignment.

720

Alignment.

This revision now requires changes to proceed.Mar 2 2017, 6:32 PM
dfeuer updated this revision to Diff 11547.Mar 3 2017, 1:58 PM
dfeuer edited edge metadata.

Expand notes.

bgamari requested changes to this revision.Mar 3 2017, 2:09 PM

The note is much better. However, a few more comments that I missed the first time through.

compiler/prelude/primops.txt.pp
2074

Why is this strictness safe?

2089

This note need to be updated. In particular be sure to address the above question.

This revision now requires changes to proceed.Mar 3 2017, 2:09 PM
dfeuer added inline comments.Mar 3 2017, 2:14 PM
compiler/prelude/primops.txt.pp
2074

I'm only pretty sure it's safe. I forgot I need to run the test suite for the stm package as well! I can try to do that tonight or tomorrow if you like. But catchRetry#, despite the suggestive name, does not actually catch exceptions. It "catches" STM retries. If its first argument throws an exception, it will just fail.

2089

I'll see what I can do later.

dfeuer added a comment.Mar 3 2017, 2:36 PM

Argh. I just realized catchRetry# probably won't work as I set it, unless
we do something a bit special with the result analysis for retry#. For now,
let's make catchRetry# lazy too. I have a vague idea for improving
treatment of retry# and raiseIO# later.

dfeuer planned changes to this revision.Mar 3 2017, 5:33 PM
dfeuer abandoned this revision.Mar 5 2017, 1:35 PM

This gets catchRetry# wrong. I'm not entirely certain that catchRetry# needs any change at all. So for now, let's just leave that as it is, and make the conservative choices for catch and catchSTM#. New differential coming up momentarily.