Skip to content

Conversation

@treeowl
Copy link
Collaborator

@treeowl treeowl commented Mar 19, 2018

  • Perform array indexing eagerly in general to avoid
    useless thunks.

  • Make munzip stricter for Array, to match the
    SmallArray instance and avoid loads of thunks.

  • Give Array and SmallArray much less inefficient
    MonadFix instances. Leaning on the instance for []
    is bad because indexing into lists is expensive, and that's
    effectively what the MonadFix instance does.

`fmap` for `Array` got its indexing all wrong. Fix that.
`>>=` for `Array` went into an infinite loop (haven't investigated
why). Replace its implementation with the working `SmallArray`
one.

Fixes haskell#92 and haskell#95
* Perform array indexing eagerly in general to avoid
  useless thunks.

* Make `munzip` stricter for `Array`, to match the
  `SmallArray` instance and avoid loads of thunks.

* Give `Array` and `SmallArray` much less inefficient
  `MonadFix` instances. Leaning on the instance for `[]`
  is bad because indexing into lists is expensive, and that's
  effectively what the `MonadFix` instance does.
@RyanGlScott
Copy link
Member

Looks promising.

One thing that's a bit unclear to me: a lot of these changes involve replacing uses of indexArray with indexArray##. Is there a general rule that one can use to determine which is better suited for one's use case?

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 20, 2018

I'm not really sure how to formulate a general rule.

case indexArray## ar j of
  (# x #) -> f x

says "Get the jth element of ar and pass it to f." On the other hand, f (indexArray ar j) says "Form a thunk that gets the jth element of ar, and pass the thunk to f." If you're in a position to know that the index is valid, you almost certainly want to perform the lookup eagerly. Demand analysis will make lookups eager when it sees that we force the values of strict computations depending on those lookups, but there are functions where that won't work at all (e.g., fmap) and others where it might work but only when enough things inline with user code (e.g., folds). The cost of a known-valid array lookup is very low in general, so it's not too bad to perform one even if it turns out we don't need the value. The cost of building and forcing a thunk is much higher.

Copy link
Member

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Gotcha. If there's not a hard-and-fast rule for when to use it, it's perhaps not worth trying to enshrine in the documentation, then.

Other parts look good to me. I've left one suggestion inline.

some sa | null sa = emptySmallArray
| otherwise = die "some" "infinite arrays are not well defined"

data ArrayStack a
Copy link
Member

@RyanGlScott RyanGlScott Mar 20, 2018

Choose a reason for hiding this comment

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

Is it possible to reuse this datatype across both SmallArray.hs and Array.hs? My (entirely untested) hypothesis is that you could turn this into:

data ArrayStack f a
  = PushArray !(f a) !(ArrayStack a)
  | EmptyStack

and put this somewhere in Data.Primitive.Internal.

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 20, 2018

I haven't exposed indexArray## because I don't really care for its name. It can get proper documentation when there's consensus on export. No, we don't want to use an array-polymorphic ArrayStack. We surely want to unpack the Array or SmallArray into the PushArray constructor, and the polymorphic version can't do that.

@RyanGlScott
Copy link
Member

We surely want to unpack the Array or SmallArray into the PushArray constructor, and the polymorphic version can't do that.

I must be missing something, as your current version doesn't UNPACK the Array/SmallArray fields, right?

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 20, 2018

I believe GHC has automatically unboxed small strict fields (i.e., word-sized ones) by default since 7.8. UNPACK these days is only really needed for unpacking wider things.

@RyanGlScott
Copy link
Member

Oops, I totally forgot that -funbox-small-strict-fields was enabled by default. In that case, please ignore my incoherent ramblings.

@treeowl treeowl merged commit ea0734a into haskell:master Mar 20, 2018
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.

2 participants