Eagerly blackhole AP_STACKs
ClosedPublic

Authored by bgamari on Jul 1 2017, 1:23 AM.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bgamari created this revision.Jul 1 2017, 1:23 AM
hsyl20 added a subscriber: hsyl20.Jul 1 2017, 3:38 AM
hsyl20 added inline comments.
rts/Apply.cmm
470

The type isn't correct and xs0 is missing.

592

s/the/they

Thanks @hsyl20! Good catches.

dfeuer added a subscriber: dfeuer.Jul 1 2017, 9:12 AM
This comment was removed by dfeuer.
bgamari updated this revision to Diff 12992.Jul 1 2017, 12:48 PM
bgamari marked an inline comment as done.
  • Fix up comment
duog added a subscriber: duog.Jul 1 2017, 2:17 PM

Macro wowbravo:

rts/Apply.cmm
514

I think this should be "walk TSO 2's stack"

simonmar added inline comments.Jul 3 2017, 3:48 AM
rts/Apply.cmm
607

I like the funky diagrams! But this doesn't fully explain what happened because there's also some sharing relationship between A and B. We know that B depends on A, because they were on the same stack, but A is also reachable by some other means that doesn't involve B.

So my attempt at summarising this is:

  1. We have two thunks, A and B, where B depends on A
  2. Both are reachable by multiple threads
  3. When evaluated, A does some private mutation using runST, e.g. it is a thunk containing a call to inplaceSum
  4. At some point, threadPaused detects that B is being evaluated by two threads
  5. Currently A is *not* being evaluated by those threads (perhaps because one thread hasn't reached A yet)
  6. B is blackholed, and A's computation is frozen as an AP_STACK
  7. Later, A's AP_STACK is evaluated by multiple threads, because it is reachable without going through B
  8. Bad things happen

So the upshot is that some computation that we think is private can become shared. Ok, I guess that's plausible.

bgamari added inline comments.Jul 3 2017, 9:28 AM
rts/Apply.cmm
607

Right, I suppose I should have made this relationship more explicit. I'll try to fix this.

bgamari updated this revision to Diff 13001.Jul 3 2017, 9:32 AM
bgamari marked an inline comment as done.
  • Fix up comment
  • testsuite: Add testcase for Trac #13615
  • Fix long line
  • Fix comments
bgamari updated this revision to Diff 13003.Jul 3 2017, 12:31 PM
  • More comment wibbles

@simonmar, how does this look now?

dfeuer requested changes to this revision.Jul 3 2017, 12:36 PM

The test doesn't seem to be wired up yet.

This revision now requires changes to proceed.Jul 3 2017, 12:36 PM
bgamari updated this revision to Diff 13004.Jul 3 2017, 12:44 PM
bgamari edited edge metadata.

Rebase

@dfeuer, oops, the test isn't supposed to be included in this diff at all. That being said, I don't know what you mean. Let's discuss this in D3696

This revision was automatically updated to reflect the committed changes.
simonmar added inline comments.Jul 4 2017, 7:17 AM
rts/Apply.cmm
509

we can say something a bit stronger: thunk B actually depends on thunk A, because thunks are deterministic (allegedly), so evaluating thunk B will always end up evaluating thunk A.. That is, assuming thunk A is not created as part of B's evaluation, which is not the case here because we believe A is shared between multiple threads.