Repository navigation
make dropWhile fusion adaptive - #327
Conversation
Now dropWhile's implementation uses stream only in case the stream fusion is already running. This is expected to reduce unnecessary re-allocation. cf. haskell#141 haskell#108
|
At a glance PR seems fine but I want to recheck that it works as advertised. I'm always become very cautious when RULES are involved. |
|
Agreed. Please tell me if I can help you in any way with rechecking. |
Shimuuar
left a comment
Shimuuar
left a comment
There was a problem hiding this comment.
Finally I got time to review this properly after I spent quite some time fighting with precision of incomplete gamma function.
It's good, I looked at generated core for simple examples and this function does work as advertised. I tested it with reproducer from #141 and this PR results in 10x speedup! (Note that dropWhile in example misses not)
| Nothing -> empty | ||
|
|
||
| -- If the argument to 'dropWhile' comes from a stream, | ||
| -- we need to avoid unnecessary allocation. |
There was a problem hiding this comment.
I think it's better to say that we want to avoid creating a new vector. Comment doesn't explain what is being allocated.
Also please add mention that new (New.unstream is body of unstream.
|
Alrighty. Adding a commit soon. |
Modified the comment for the RULE "dropWhile/unstream [Vector]" in order to make the intention of the implementation there clear.
|
Now I've committed and pushed a change to the comment. If you'd like a rebase, squash, etc, please tell me. |
|
Thanks! I'll just squash merged PR. |
* make dropWhile fusion adaptive Now dropWhile's implementation uses stream only in case the stream fusion is already running. This is expected to reduce unnecessary re-allocation. cf. #141 #108 * Adaptive dropWhile Implementation: Implementation comment modified Modified the comment for the RULE "dropWhile/unstream [Vector]" in order to make the intention of the implementation there clear.
2 participants
Now dropWhile's implementation uses stream
only in case the stream fusion is already running.
This is expected to reduce unnecessary re-allocation.
This is an improvement of #146, which does not admit stream fusion in any case.
I've executed the tests included in the package with GHC 8.6.5 and 8.8.3.
If you wish to have other tests done, please tell me.
cf. #141
#108