Use functions from newer primitive and primitive-unlifted #63
Conversation
The implementation UnliftedArray in primitive-unlifted-2.1 penalizes the creation of an uninitialized unlifted array. When shrinking and resizing unlifted arrays, there are primitives that we can use to avoid this. Also, primitive itself now has shims for common operations on PrimArray, so this commit also cleans up the Shim module.
There was a problem hiding this comment.
I'd change line 142 to:
-- | Resize a mutable array without growing it. It may be shrunk in place.
There was a problem hiding this comment.
I'd still like line 142 changed (to add " It may be shrunk in place."). :)
| {-# INLINE shrink #-} | ||
| shrink !arr !n = do | ||
| shrinkSmallMutableArray arr n | ||
| pure arr |
There was a problem hiding this comment.
Excellent! I'd do these 4 lines for PrimArray also, using shrinkMutablePrimArray, thanks.
There was a problem hiding this comment.
Good idea. I've done this in 95e48d0.
| -- Note [Shrinking Unlifted Arrays] | ||
| -- ================================ | ||
| -- This implementation copies the array rather than freezing it in place. | ||
| -- But at least it is able to avoid assigning all of the elements to | ||
| -- a nonsense value before replacing them with memcpy. |
There was a problem hiding this comment.
Wow, very clever! But I think you want to apply this to lifted arrays also, right? Including defining a shrink using cloneMutableArray, and also in Shim.hs changing resizeArray to be like resizeUnliftedArray. Then maybe deleting or commenting out the default shrink?
There was a problem hiding this comment.
Good idea. Added this change in 95e48d0.
Co-authored-by: Dave Barton <[email protected]>
Co-authored-by: Dave Barton <[email protected]>
Co-authored-by: Dave Barton <[email protected]>
|
I've made the changed you suggested. I've also gone through and remove several of the default implementations for things. As I was looking at the code, I realized that the default implementations of all the functions related to shrinking and in-place freezing just made it hard to reason about what was going on, and those are operations where I really want to know what's going on. Let me know if there are any other concerns. |
|
Ping? There are just a very few corrections left. Maybe it's better to make these now while your memory is still fresh. Also I'd like to use the new library. :) Thanks! |
Co-authored-by: Dave Barton <[email protected]>
Co-authored-by: Dave Barton <[email protected]>
Co-authored-by: Dave Barton <[email protected]>
Co-authored-by: Dave Barton <[email protected]>
Co-authored-by: Dave Barton <[email protected]>
|
Sorry, I had totally missed the notification when you left a review. I've merged all of your suggestions into the branch. Anything else, or should I go ahead and squash this down? |
|
Looks great! I think the only thing left is the comment in Class.hs, adding "It may be shrunk in place.": I'd change line 142 to: |
The implementation UnliftedArray in primitive-unlifted-2.1 penalizes the creation of an uninitialized unlifted array. When shrinking and resizing unlifted arrays, there are primitives that we can use to avoid this.
Also, primitive itself now has shims for common operations on PrimArray, so this commit also cleans up the Shim module.