Skip to content

Conversation

@treeowl
Copy link
Collaborator

@treeowl treeowl commented Mar 23, 2018

Closes #102

@treeowl treeowl force-pushed the worker-wrapper branch 2 times, most recently from e45a301 to 47d2765 Compare March 23, 2018 06:28
@treeowl
Copy link
Collaborator Author

treeowl commented Mar 23, 2018

The Core looks rather better, IMO (see, e.g., what this does to fmap). I don't have benchmarks to prove it's good, but I think we should merge this anyway.

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 23, 2018

Another option is to write runArray in this manner (which is smaller and simpler), and use that to implement createArray. The inlining decisions end up a little different; I'm not sure which would really be better. But we likely should export runArray anyway.

@RyanGlScott
Copy link
Member

Forgive me for being dense, but I'm still a bit unclear (from the comments, at least) how this is improving the interaction w.r.t. worker-wrapper. Can you expand on the comments a bit to clarify?

Also, if I understand correctly, this will make creating empty arrays slightly slower, yes? If so, is there an alternative way to create an empty array that is faster? We should point in out in the Haddocks if so.

@cartazio
Copy link
Contributor

i guess another way of stating Ryan's question is:

what is a small example of what worker wrapper will facilitate here?

An important detail to keep in mind with Primitive is that its foremost a toolkit for mutable data structures, and fusion-y optimizations tend to be very low ROI in those cases. We should provide all the right primitives for supporting easy optimization in client libraries (like vector), but that doesn't mean we should push fusion into this layer?

I'm open to being educated / convinced about this, but could you help Ryan and Myself understand the goal / code improvements this will facilitate and some toy example where the change is high impact/measurable?

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 23, 2018

The fact that immutable arrays aren't the prime directive doesn't mean they should get short shrift! I'm not really doing anything fancy here. Suppose I write f (fmap g ary), where f is strict (it's not just sticking the array in an array or something silly like that). We don't generally want fmap to actually produce an Array here. We want to produce an Array# and pass that straight to the worker for f. The problem with the current implementation is that it applies the Array constructor inside an ST action. runRW# doesn't inline till Core Prep, when it's too late to unbox anything. By moving the Array constructor application outside runRW#, we let GHC erase it.

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 23, 2018

To be clear,

  1. I have absolutely no desire to try to add any fusion framework to Array.
  2. That doesn't mean there shouldn't be any fusion rules.
  3. Nothing in this PR is very fusiony, except in the sense that GHC "fuses" away wrappers when it can.

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 23, 2018

@RyanGlScott, yes, this will make creating empty arrays with createArray a bit slower. Instead of pointing to the same Array, separate invocations will point to different Arrays that all point to the same Array#. Since createArray is internal, I don't think there's anything to be gained from an extra empty export.

@RyanGlScott
Copy link
Member

Since createArray is internal

Oops, I missed that part, so that makes that concern pretty much moot :)

@treeowl treeowl force-pushed the worker-wrapper branch 5 times, most recently from 4415300 to c41af1c Compare March 25, 2018 17:17
* Let GHC can erase `Array` constructors in many cases.
* Add a `runArray` function like the ones in `array` and `vector`.

Closes haskell#102
@treeowl treeowl force-pushed the worker-wrapper branch 3 times, most recently from c47d53f to b233062 Compare March 25, 2018 19:45
@treeowl treeowl merged commit c40a0d5 into haskell:master Mar 25, 2018
@treeowl treeowl deleted the worker-wrapper branch March 25, 2018 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants