Skip to content

Buffering - #24

Closed
treeowl wants to merge 4 commits into
haskell:masterfrom
treeowl:buffering
Closed

Buffering#24
treeowl wants to merge 4 commits into
haskell:masterfrom
treeowl:buffering

Conversation

@treeowl

@treeowl treeowl commented Jun 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

  • 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

@treeowl
treeowl force-pushed the buffering branch 2 times, most recently from a15f6de to 0394965 Compare June 3, 2018 01:55
bgamari
bgamari reviewed Jun 3, 2018
View reviewed changes
Comment thread Control/Parallel/Strategies.hs Outdated
bgamari
bgamari approved these changes Jun 3, 2018
View reviewed changes

@bgamari bgamari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite nice!

Comment thread Control/Parallel/Strategies.hs Outdated
@treeowl
treeowl force-pushed the buffering branch 3 times, most recently from eaf6598 to a70ff96 Compare June 3, 2018 03:03
@treeowl

treeowl commented Jun 3, 2018

Copy link
Copy Markdown
Contributor Author

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

treeowl commented Jun 3, 2018

Copy link
Copy Markdown
Contributor Author

@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
simonmar requested changes Jun 4, 2018
View reviewed changes

@simonmar simonmar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Control/Parallel/Strategies.hs Outdated
Comment thread Control/Parallel/Strategies.hs Outdated
Comment thread Control/Parallel/Strategies.hs
Comment thread Control/Parallel/Strategies.hs
Borgvall and others added 3 commits June 17, 2018 21:39
@Borgvall @treeowl
Add test for corner case rparWith r0
d2e6417 
It does not pass with the current implementation of `rparWith`.
@treeowl
Restore rparWith behavior
9957b3e 
* Lift the result to avoid always reducing to WHNF.

* Rewrite the documentation of `rparWith`.

Fixes haskell#35
@treeowl
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
More conservative
6f7463f 
Don't try to change lots of `RULES` and inlining in this PR;
keep it more confined.
@treeowl

treeowl commented Jun 18, 2018

Copy link
Copy Markdown
Contributor Author

@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:

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

@simonmar

simonmar commented Jun 18, 2018

Copy link
Copy Markdown
Member

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)

@konsumlamm

konsumlamm commented Mar 14, 2025

Copy link
Copy Markdown
Collaborator

I think we should replace evalBuffer by buffering (but keep the type), since the current behaviour (evaluating every element at least to WHNF, regardless of the strategy) is quite unexpected.

@treeowl would you mind updating the PR? Otherwise, I can take over.

@treeowl

treeowl commented Mar 14, 2025

Copy link
Copy Markdown
Contributor Author

I can try to page it back in; it's been a while since I wrote this.

@konsumlamm konsumlamm mentioned this pull request May 3, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simonmar simonmar simonmar requested changes
@konsumlamm konsumlamm konsumlamm left review comments
@bgamari bgamari bgamari approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

parBuffer smells wrong

5 participants

@treeowl @simonmar @konsumlamm @bgamari @Borgvall