Buffering #24

treeowl · 2018-06-02T18:30:06Z

Add buffering to buffer compositionally.
Manually deforest evalBuffer and parBuffer.
Add more rules for evalBuffer and parBuffer.

This PR is layered on another; I could disentangle them if necessary.

Closes #23

bgamari · 2018-06-03T02:22:51Z

Control/Parallel/Strategies.hs

+-- lazily, with runEval between steps. runEval is really just
+-- unsafeDupablePerformIO. If we used an array-based queue, and
+-- a thunk representing the tail of the result gets duplicated, then
+-- we could scramble things quite badly. A pure queue can't run into


Instead of "we could scramble things quite badly" perhaps say:

We could end up pushing a value more than once, potentially scrambling the queue.

bgamari

Quite nice!

bgamari · 2018-06-03T02:28:06Z

Control/Parallel/Strategies.hs

+-- it sparks computations to evaluate list elements at least to weak
+-- head normal form, disregarding a strategy argument 'r0'.
+--
+-- > parBuffer n strat = parBuffer n (rseq `dot` strat)


Much better!

treeowl · 2018-06-03T14:23:35Z

I'd actually prefer to do things a little differently with all the buffering functions. In particular, I think that buffering n xs should fill the buffer with n elements immediately, rather than doing nothing until the result is forced to WHNF. But consistency among these functions seems pretty important.

treeowl · 2018-06-03T22:44:41Z

@simonmar, for the sake of consistency with the rest of the module, I don't think the poorly named buffering function should actually be added. Rather, I think it should replace evalBuffer. We could use rewrite rules internally to optimize evalBuffer (rseq `dot` strat), but I'm not sure that's really worth the trouble. On the other hand, that would be a breaking change.

simonmar

For this kind of change I think it's pretty important to get some benchmarks to demonstrate that at the least it doesn't cause any regressions. We have nofib/parallel, and you'll need a machine with at least 8 cores.

I'm OK with adding things to the API, but we should be careful about breaking changes because this API is used in my book.

simonmar · 2018-06-04T08:03:34Z

Control/Parallel/Strategies.hs

@@ -340,6 +342,7 @@ type SeqStrategy a = Control.Seq.Strategy a
 --
 r0 :: Strategy a
 r0 x = return x
+{-# INLINE [1] r0 #-}


Why? Please add a comment.

simonmar · 2018-06-04T08:03:46Z

Control/Parallel/Strategies.hs

@@ -394,7 +397,7 @@ rpar  x = Eval $ IO $ \s -> spark# x s
 #else
 rpar  x = case (par# x) of { _ -> Done x }
 #endif
-{-# INLINE rpar  #-}
+{-# INLINE [1] rpar  #-}


simonmar · 2018-06-04T08:08:50Z

Control/Parallel/Strategies.hs

+    tieConses :: (a -> Eval b) -> [a] -> [b]
+    tieConses strat = foldr go []
+      where
+        go x r = runEval ((:r) <$> strat x)


This makes my brain hurt.

simonmar · 2018-06-04T08:09:02Z

Control/Parallel/Strategies.hs

 {-# RULES
 "evalBuffer/rseq"  forall n . evalBuffer  n rseq = evalBufferWHNF n
-"parBuffer/rseq"   forall n . parBuffer   n rseq = parBufferWHNF  n


I think it was pretty important to optimise this case, I'm worried this might be a regression.

Ideally, users wouldn't write parBuffer n rseq, because that's just the same as parBuffer n r0. I think the right way to catch this (and other such) is probably to write rules for the underlying operations:

runEval (r0 x) ==> x runEval (rseq x) ==> x runEval (rpar x) ==> x

Making the higher-order combinators INLINABLE should then expose things sufficiently to these low-level rules.

Note if you're following by email: I edited the above comment substantially.

I added a comment explaining what tieConses does. I don't think it's actually complicated. I wrote it with foldr so we can consider making the whole thing INLINABLE and get list fusion on one side. Perhaps it would be easier to see what's going on if you write it with explicit recursion?

Could you explain why parBuffer n rseq should be the same as parBuffer n r0? That seems counter-intuitive to me.

It's really because parBuffer isn't compositional. A better way is to use the runEval (rpar x) = x rule so the strategy is simplified before being passed to buffering. I can change the PR. I'm wondering if you would be okay with redefining evalBuffering to buffering; it's a breaking change, but makes things much more uniform across the module.

@simonmar, that was actually more fallout from my misunderstanding of rparWith. Ugh. parBuffer n rseq is not the same as parBuffer n r0.

@simonmar, um, sorry, what I just said was a bit confused. The current behavior of parBuffer actually equates parBuffer n r0 to parBuffer n rseq. My implementation in terms of buffering and rparWith does not, once rparWith is fixed.

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

* Lift the result to avoid always reducing to WHNF. * Rewrite the documentation of `rparWith`. Fixes haskell#35

* Add `buffering` to buffer compositionally. * Redefine `parBuffer` in terms of `buffering`. * Manually deforest `evalBuffer`. * Add more rules for `evalBuffer`.

Don't try to change lots of `RULES` and inlining in this PR; keep it more confined.

treeowl · 2018-06-18T02:53:23Z

@simonmar, I've made the changes much more conservative, which I'm hoping will make it easier to merge. In particular, I have ideas about reworking the way we deal with RULES and inlining, but they don't really belong here. I can likely borrow an 8-core machine to run the benchmarks; I'll have to try that later in the week. I continue to wonder what you think about the proposed semantic changes:

Make evalBuffer only evaluate as far as the passed strategy says, rather than to WHNF. (This is not incorporated into the PR).
Make parBuffer spark computations that only evaluate as far as the passed strategy says. (This is incorporated into the PR).

simonmar · 2018-06-18T07:55:38Z

Hi @treeowl - I'm inclined to be very conservative with merging further changes to this package, given that we've all become confused at one time or another about how things are supposed to work. So let me propose that before merging any further PRs, especially unforced ones, we should

make the test suite work with Travis
do some benchmarking for every PR to ensure that it doesn't regress parallel performance (I'm quite concerned that we haven't done any benchmarking so far, and the whole reason this package exists is for performance)

treeowl force-pushed the buffering branch 2 times, most recently from a15f6de to 0394965 Compare June 3, 2018 01:55

bgamari reviewed Jun 3, 2018

View reviewed changes

bgamari approved these changes Jun 3, 2018

View reviewed changes

treeowl force-pushed the buffering branch 3 times, most recently from eaf6598 to a70ff96 Compare June 3, 2018 03:03

treeowl force-pushed the buffering branch from a70ff96 to 3548e53 Compare June 3, 2018 22:35

treeowl force-pushed the buffering branch from 3548e53 to 134c4e3 Compare June 4, 2018 01:40

simonmar requested changes Jun 4, 2018

View reviewed changes

treeowl force-pushed the buffering branch from 134c4e3 to 8aeb510 Compare June 4, 2018 13:40

Borgvall and others added 3 commits June 17, 2018 21:39

Add test for corner case rparWith r0

d2e6417

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

Restore rparWith behavior

9957b3e

* Lift the result to avoid always reducing to WHNF. * Rewrite the documentation of `rparWith`. Fixes haskell#35

Add a compositional buffering strategy

4bc8230

* Add `buffering` to buffer compositionally. * Redefine `parBuffer` in terms of `buffering`. * Manually deforest `evalBuffer`. * Add more rules for `evalBuffer`.

treeowl force-pushed the buffering branch from 8aeb510 to 4bc8230 Compare June 18, 2018 02:36

More conservative

6f7463f

Don't try to change lots of `RULES` and inlining in this PR; keep it more confined.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffering #24

Buffering #24

treeowl commented Jun 2, 2018 •

edited

Loading

bgamari Jun 3, 2018 •

edited

Loading

bgamari left a comment

bgamari Jun 3, 2018

treeowl commented Jun 3, 2018

treeowl commented Jun 3, 2018

simonmar left a comment

simonmar Jun 4, 2018

simonmar Jun 4, 2018

simonmar Jun 4, 2018

simonmar Jun 4, 2018

treeowl Jun 4, 2018 •

edited

Loading

treeowl Jun 4, 2018

treeowl Jun 4, 2018

simonmar Jun 8, 2018

treeowl Jun 8, 2018

treeowl Jun 18, 2018

treeowl Jun 18, 2018

treeowl commented Jun 18, 2018

simonmar commented Jun 18, 2018

Buffering #24

Are you sure you want to change the base?

Buffering #24

Conversation

treeowl commented Jun 2, 2018 • edited Loading

bgamari Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

bgamari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treeowl commented Jun 3, 2018

treeowl commented Jun 3, 2018

simonmar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treeowl Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treeowl commented Jun 18, 2018

simonmar commented Jun 18, 2018

treeowl commented Jun 2, 2018 •

edited

Loading

bgamari Jun 3, 2018 •

edited

Loading

treeowl Jun 4, 2018 •

edited

Loading