-
Notifications
You must be signed in to change notification settings - Fork 273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eager seq ix #965
base: master
Are you sure you want to change the base?
Eager seq ix #965
Conversation
Use `!?` for vector `ix` rather than manual range calculations. Once [this PR](haskell/vector#347) lands, that will also make the lookups eager, as they should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eager here means that we don't retain the original sequence/vector, i.e. f x
is f <element value>
and not f <think to lookup element>
?
If my understanding is right, could you amend commit message(s) with that, so it would be clearer what these changes do.
ix i f m = case Seq.lookup i m of | ||
Just x -> f x <&> \x' -> Seq.update i x' m | ||
Nothing -> pure m | ||
#else | ||
ix i f m | ||
| 0 <= i && i < Seq.length m = f (Seq.index m i) <&> \a -> Seq.update i a m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is here impossible to seq
things to avoid thunk, Would
let x = Seq.index m i `seq` (f x <&> ...)
do anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using seq
with index
would force too much, forcing not only the lookup but also the element itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. as later then 0.5.8
containers
are bundled with GHC-8.2, I don't think we need to do here anything. (If someone have problems due this lazyness, and they could upgrade GHC or/and containers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phadej Do you mean we shouldn't worry about the leak for older containers
? I agree. Trying to plug it in that context will be very inefficient when f
is actually strict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add changelog entry. If you explain what this change means there (not just saying eager seq ix), then we don't need to repeat the explanation in commit messages.
@phadej For |
What is the status of this PR? If I'm reading correctly, this is very nearly done modulo documentation/changelog concerns. (I'm asking since I'm doing a roundup of recent PRs before an upcoming |
Haven't looked at this in ages. Can try to look today. |
ping @treeowl |
Pong! I will try to look at this today. |
ix
look up eagerly forSeq
.ix
use!?
for vectors, which should eventually make those lookups eager too.