initial draft oft hold_max with heuristic#657
Conversation
|
this is now ready for some feedback @djc |
|
would love some feedback on this |
djugei
force-pushed
the
heuristic
branch
5 times, most recently
from
e72c3c9 to
d0715e4
Compare
|
can i get some feedback on this? |
|
rebased to 1.18 |
will do before a merge, i am keeping it in separate commits for now to make reviews easier (for example its somewhat easier to ignore the code movements by just not looking at that commit etc) i will make the requested changes over the weekend probably |
djugei
force-pushed
the
heuristic
branch
3 times, most recently
from
b408abb to
23a4b1d
Compare
|
I have done all(-1) of the requested changes, also split out the bug fix into a separate commit while squashing all other changes. |
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
Thanks for working on this, and sorry for the very slow feedback -- will try to be faster for the next one.
One more nit: you have a bunch of unidiomatic variable names, oldpos, maxpos. All of these should have underscores (like old_pos). Also suggest prev_ instead of old_ prefixes.
src/iter.rs
Outdated
| // % here is a purely for the optimizer | ||
| let head: usize = usize::from(self.head) % self.vals.len(); |
There was a problem hiding this comment.
Please drop the unnecessary type annotation. Unless you've seen actual benchmark data where the % matters, let's drop the optimization.
There was a problem hiding this comment.
i have decided doubling down on this, i am altering the docs/comments to talk more about invariants.
i have just rechecked, it does fully eliminate bounds checks. while bounds checks are not that performance intensive, it also eliminates the entire panic path, which has a positive impact on code size. Also it just feels right to feed the compiler additional information.
i was somewhat tempted to use std::hint::assert_unchecked, which has much the same effect, but also eliminates the modulo calculations. i opted to not introduce unsafe code, though its correctness can be somewhat trivially asserted.
djugei
force-pushed
the
heuristic
branch
2 times, most recently
from
66002de to
c7886d7
Compare
|
integrated feedback |
no worries, we all do this in our free time, and the interactions with this project have been plesant so far, much worse is possible :D off topic but also thanks for cutting the dialoguer release that outdated dependency has been bugging me for quite a while now, i try to only ever have one version of a lib in my dependency tree. |
|
sorry for the wait, have resolved the issues. i have also rebased to latest master. |
|
Thanks -- can you fix up the clippy issue? I will do a pass after this is merged to clean up some things. |
|
the clippy issue is literally a change you requested, but i have fixed it now. |
On the first seek it gets enabled, displaying the progress as the max of the last 10 updates. If there ever are more than 5 consecutive reads and writes without seek it gets disabled again, keeping the performance impact low.
2 participants
alternative or update to #642
In BufRead/AsyncBufRead fill_buf does not logically consume the reader/advance progress, only a call to consume does. this was the wrong way around for the AsyncBufRead implementation, i have fixed that in this pr.