Use runRW# to implement unsafeInterleaveIO
AbandonedPublic

Authored by dfeuer on Mar 9 2017, 5:40 PM.

Details

Summary

Instead of holding on to the past, let's start a new timeline.
This seems a lot cleaner, and may even allow inlining.

Fixes Trac Trac #13405

dfeuer created this revision.Mar 9 2017, 5:40 PM
dfeuer updated this revision to Diff 11670.Mar 10 2017, 1:45 AM
  • Simplify further
dfeuer planned changes to this revision.Mar 10 2017, 2:54 AM

One concern edsko raised is that GHC could possibly perform the interleaved I/O early. It's a bit hard to say, because io_hack_reqd looks a bit surprising. Somehow it says it's not required for any primop? Or something? I don't understand.

dfeuer requested review of this revision.Mar 10 2017, 3:18 AM
bgamari accepted this revision.Mar 10 2017, 3:41 PM

Looks reasonable to me but in terrible need of an explanatory comment.

This revision is now accepted and ready to land.Mar 10 2017, 3:41 PM
carter requested changes to this revision.Mar 11 2017, 7:22 AM
carter added a subscriber: carter.
  1. where's definition of pure?
  2. I'm not sure if the new comments are enough ... should document the ways it should work / preconditions.
  3. this change is for master or 8.2? I'm guessing master ?
  1. the definition for the st monad needs to be revised too probably ?
  1. do we have any tests that could distinguish between this and the old definition? Might be good to exercise any expected changes such as the exceptions in the presence of optimization bit
This revision now requires changes to proceed.Mar 11 2017, 7:22 AM

If it's actually wrong, we should be able to add a test to prove that.
Furthermore: I think it would be cleaner to express the dependency some
other way, if possible, than by reusing the state token as a state token.
Is there another way we can "use" it?

In D3308#95782, @dfeuer wrote:

If it's actually wrong, we should be able to add a test to prove that.

Not necessarily, since we might not be able to convince the compiler to do a transformation that it could potentially do (perhaps only in the future). But if we can write a test that will fail reliably under the wrong definition, then it would be good to have too.

But a Note, or even extending the user-facing documentation, is much more important, since it better defines the expected semantics of unsafeInterleaveIO, and would prevent someone from going down this wrong path again in the future.

Furthermore: I think it would be cleaner to express the dependency some
other way, if possible, than by reusing the state token as a state token.
Is there another way we can "use" it?

It seems quite clear to me; the action m can only run after whatever comes before unsafeInterleaveIO m because it needs the state token s that was passed to unsafeInterleaveIO m. What cleaner way could there be?

Furthermore: I think it would be cleaner to express the dependency some
other way, if possible, than by reusing the state token as a state token.
Is there another way we can "use" it?

It seems quite clear to me; the action m can only run after whatever comes before unsafeInterleaveIO m because it needs the state token s that was passed to unsafeInterleaveIO m. What cleaner way could there be?

All right; I guess you're right. However, I still think there's room for improvement here. First off, I think we only need to be careful about inlining r; I don't see how inlining unsafeDupablePerformIO itself could cause trouble. Second, I figure we may be able to improve it further using a generalization of runRW#. We win generally with runRW# by inlining it, but only in core prep. We should be able to do something similar using a variant that applies a state token we provide. Indeed, I wonder if we could make it fully general:

inlineLate# :: forall (r1 :: RuntimeRep) (r2 :: RunTimeRep) (arg :: TYPE r1) (res :: TYPE r2) .
   (arg -> res) -> arg -> res

This would have a compulsory unfolding applied in core prep, which should prevent its absurd representation polymorphism from causing trouble.

dfeuer abandoned this revision.Mar 13 2017, 9:19 AM
In D3308#95806, @dfeuer wrote:

Second, I figure we may be able to improve it further using a generalization of runRW#. We win generally with runRW# by inlining it, but only in core prep. We should be able to do something similar using a variant that applies a state token we provide.

I see what you're getting at now, though I'm not sure how much there is to win. The generated code for unsafePerformIO was obviously bad, since it always allocated a thunk that always got evaluated immediately. But the return value of unsafeInterleaveIO is usually not evaluated immediately, so it's not clear to me off-hand whether we would save an allocation in the normal case. (Plus unsafeInterleaveIO is far more rare than unsafePerformIO, and the uses usually involve some expensive IO anyways.)