Restore `rparWith r0` documented behaviour by Borgvall · Pull Request #34 · haskell/parallel

Borgvall · 2018-06-10T11:20:28Z

In addition I have added a test case, to prevent this regression in the future.

It does not pass with the current implementation of `rparWith`.

I know the implementation of rparWith looks ugly, but the lazy 'Lift' box is necessary, to make sure rparWith has no built-in rseq.

treeowl

This is exactly the same without optimization, but broken with optimization.

Borgvall · 2018-06-10T17:16:58Z

No it's not the order of the output of the test program changes, depending on the implementation of rparWith. See the following bash output on my Fedora 27 workstation with GHC 8.0.2.

[johannes@fedorawork parallel]$ cp test\rparWith_r0.hs .
[johannes@fedorawork parallel]$ ghc -O0 -threaded -fforce-recomp rparWith_r0.hs  
[1 of 4] Compiling Control.Parallel ( Control/Parallel.hs, Control/Parallel.o )
[2 of 4] Compiling Control.Seq      ( Control/Seq.hs, Control/Seq.o )
[3 of 4] Compiling Control.Parallel.Strategies ( Control/Parallel/Strategies.hs, Control/Parallel/Strategies.o )
[4 of 4] Compiling Main             ( rparWith_r0.hs, rparWith_r0.o )
Linking rparWith_r0 ...
[johannes@fedorawork parallel]$ ./rparWith_r0 
Must be printed first.
Must be printed at the end.
[johannes@fedorawork parallel]$ git revert fdce43b84
[restore-rparWith-r0 763661f] Revert "Restore the documented behaviour of `rparWith r0`"
 1 file changed, 1 insertion(+), 8 deletions(-)
[johannes@fedorawork parallel]$ ghc -O0 -threaded -fforce-recomp rparWith_r0.hs 
[1 of 4] Compiling Control.Parallel ( Control/Parallel.hs, Control/Parallel.o )
[2 of 4] Compiling Control.Seq      ( Control/Seq.hs, Control/Seq.o )
[3 of 4] Compiling Control.Parallel.Strategies ( Control/Parallel/Strategies.hs, Control/Parallel/Strategies.o )
[4 of 4] Compiling Main             ( rparWith_r0.hs, rparWith_r0.o )
Linking rparWith_r0 ...
[johannes@fedorawork parallel]$ ./rparWith_r0 
Must be printed at the end.
Must be printed first.
[johannes@fedorawork parallel]$ ghc -O2 -threaded -fforce-recomp rparWith_r0.hs  
[1 of 4] Compiling Control.Parallel ( Control/Parallel.hs, Control/Parallel.o )
[2 of 4] Compiling Control.Seq      ( Control/Seq.hs, Control/Seq.o )
[3 of 4] Compiling Control.Parallel.Strategies ( Control/Parallel/Strategies.hs, Control/Parallel/Strategies.o )
[4 of 4] Compiling Main             ( rparWith_r0.hs, rparWith_r0.o )
Linking rparWith_r0 ...
[johannes@fedorawork parallel]$ ./rparWith_r0 
Must be printed at the end.
Must be printed first.
[johannes@fedorawork parallel]$ git checkout v3.2.1.0
Hinweis: Checke 'v3.2.1.0' aus.

Sie befinden sich im Zustand eines 'lösgelösten HEAD'. Sie können sich
umschauen, experimentelle Änderungen vornehmen und diese committen, und
Sie können alle möglichen Commits, die Sie in diesem Zustand machen,
ohne Auswirkungen auf irgendeinen Branch verwerfen, indem Sie einen
weiteren Checkout durchführen.

Wenn Sie einen neuen Branch erstellen möchten, um Ihre erstellten Commits
zu behalten, können Sie das (jetzt oder später) durch einen weiteren Checkout
mit der Option -b tun. Beispiel:

  git checkout -b <neuer-Branchname>

HEAD ist jetzt bei dbb8d9e... Fix minor typo in .cabal file
[johannes@fedorawork parallel]$ ghc -O2 -threaded -fforce-recomp rparWith_r0.hs 
[1 of 4] Compiling Control.Parallel ( Control/Parallel.hs, Control/Parallel.o )
[2 of 4] Compiling Control.Seq      ( Control/Seq.hs, Control/Seq.o )
[3 of 4] Compiling Control.Parallel.Strategies ( Control/Parallel/Strategies.hs, Control/Parallel/Strategies.o )
[4 of 4] Compiling Main             ( rparWith_r0.hs, rparWith_r0.o )
Linking rparWith_r0 ...
[johannes@fedorawork parallel]$ ./rparWith_r0 
Must be printed first.
Must be printed at the end.
[johannes@fedorawork parallel]$ ghc -O0 -threaded -fforce-recomp rparWith_r0.hs  
[1 of 4] Compiling Control.Parallel ( Control/Parallel.hs, Control/Parallel.o )
[2 of 4] Compiling Control.Seq      ( Control/Seq.hs, Control/Seq.o )
[3 of 4] Compiling Control.Parallel.Strategies ( Control/Parallel/Strategies.hs, Control/Parallel/Strategies.o )
[4 of 4] Compiling Main             ( rparWith_r0.hs, rparWith_r0.o )
Linking rparWith_r0 ...
[johannes@fedorawork parallel]$ ./rparWith_r0 
Must be printed first.
Must be printed at the end.

treeowl · 2018-06-11T03:27:30Z

Instead of reverting 9ea4c07, define rparWith like this:

rparWith s a = do
  l <- rpar r
  return (case l of Lift x -> x)

  where
    r = runEval (Lift <$> s a)

data Lift a = Lift a

That's actually the same as before, but doesn't have the realWorld# invocation that led to #17.

treeowl · 2018-06-11T03:31:34Z

The documentation for rparWith also really needs to be clarified to explain how rparWith s is different from rpar `dot` s. The business about exiting the Eval monad is also fairly bogus and should probably be removed.

simonmar · 2018-06-11T08:08:15Z

@Borgvall thanks for spotting the problem and for the test case!

Yes we also need to fix the documentation. Personally I'd also like to have a better understanding of where the original implementation went wrong - @treeowl is there a succinct way to describe the problem that we could put in a comment?

It would be nice if parBuffer worked the same way as rparWith, it seems strange to me that parBuffer r0 should be the same as parBuffer rseq.

treeowl · 2018-06-11T10:27:25Z

If we adopt my proposed evalBuffer = buffering, then parBuffer n s = evalBuffer n (rparWith s) should do the trick.

simonmar · 2018-06-14T09:05:48Z

We still need to get this fixed. @Borgvall would you be able to update the PR with @treeowl's suggestion to use runEval?

@treeowl is there any way to add a test so that the bad behaviour with realWorld# doesn't reappear?

treeowl · 2018-06-14T20:46:06Z

@simonmar, I opened #36 to replace @Borgvall's second commit with one that uses runEval and fixes the documentation.

Borgvall · 2018-06-15T18:04:08Z

I tried to reproduce the issue #17 with this branch, using the example code of the report, where I replaced parList with parTraversable. With GHC (8.0.2), threaded runtime and "-O2" it created 1000 sparks as I expect. Probably #17 was not fixed, by changing rparWith? Or does it only affects newer GHCs?

module Main where

import Control.Parallel.Strategies

main :: IO ()
main = do let xs = take 1000 $ product . take 1000 . repeat <$> [1..]
              x  = product (xs `using` parTraversable rseq)
          putStrLn (show x)

treeowl · 2018-06-15T18:07:05Z

#17 was fixed by using unsafeDupablePerformIO (i.e., runRW#) instead of manually passing realWorld#.

treeowl · 2018-06-15T18:08:33Z

I think this PR can be closed. It's subsumed by #36.

Borgvall · 2018-06-15T20:17:05Z

tests/rparWith_r0.hs

+  LazyBox printWhenEvaluated <- evaluate $ runEval $ do
+    let printWhenEvaluated = unsafePerformIO $ putStrLn "Must be printed at the end."
+    sparkedResult <- rparWith r0 printWhenEvaluated
+    return $ LazyBox sparkedResult


Since #3 has been accepted, one can simplify this mess after merge to master:

printWhenEvaluated <- unsafePerformIO (putStrnLn "Must be printed at the end.") `usingIO` rparWith r0

Borgvall added 2 commits June 10, 2018 12:18

Add test for corner case rparWith r0

f441bb7

It does not pass with the current implementation of `rparWith`.

Restore the documented behaviour of rparWith r0

fdce43b

I know the implementation of rparWith looks ugly, but the lazy 'Lift' box is necessary, to make sure rparWith has no built-in rseq.

Borgvall mentioned this pull request Jun 10, 2018

Regression: The behaviour of rparWith r0 has changed #35

Closed

treeowl reviewed Jun 10, 2018

View reviewed changes

Borgvall commented Jun 15, 2018

View reviewed changes

Borgvall closed this Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore `rparWith r0` documented behaviour#34

Restore `rparWith r0` documented behaviour#34
Borgvall wants to merge 2 commits intohaskell:masterfrom
Borgvall:restore-rparWith-r0

Borgvall commented Jun 10, 2018

Uh oh!

treeowl left a comment

Uh oh!

Borgvall commented Jun 10, 2018

Uh oh!

treeowl commented Jun 11, 2018

Uh oh!

treeowl commented Jun 11, 2018

Uh oh!

simonmar commented Jun 11, 2018

Uh oh!

treeowl commented Jun 11, 2018

Uh oh!

simonmar commented Jun 14, 2018

Uh oh!

treeowl commented Jun 14, 2018

Uh oh!

Borgvall commented Jun 15, 2018

Uh oh!

treeowl commented Jun 15, 2018

Uh oh!

treeowl commented Jun 15, 2018

Uh oh!

Borgvall Jun 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Borgvall commented Jun 10, 2018

Uh oh!

treeowl left a comment

Choose a reason for hiding this comment

Uh oh!

Borgvall commented Jun 10, 2018

Uh oh!

treeowl commented Jun 11, 2018

Uh oh!

treeowl commented Jun 11, 2018

Uh oh!

simonmar commented Jun 11, 2018

Uh oh!

treeowl commented Jun 11, 2018

Uh oh!

simonmar commented Jun 14, 2018

Uh oh!

treeowl commented Jun 14, 2018

Uh oh!

Borgvall commented Jun 15, 2018

Uh oh!

treeowl commented Jun 15, 2018

Uh oh!

treeowl commented Jun 15, 2018

Uh oh!

Borgvall Jun 15, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants