Skip to content

initial draft oft hold_max with heuristic #657

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

djugei
Copy link
Contributor

@djugei djugei commented Sep 9, 2024

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.

@djugei
Copy link
Contributor Author

djugei commented Sep 13, 2024

this is now ready for some feedback @djc

@djugei
Copy link
Contributor Author

djugei commented Oct 11, 2024

would love some feedback on this

@djugei djugei force-pushed the heuristic branch 5 times, most recently from e72c3c9 to d0715e4 Compare February 5, 2025 20:25
@djugei
Copy link
Contributor Author

djugei commented Jul 6, 2025

can i get some feedback on this?

@djugei
Copy link
Contributor Author

djugei commented Jul 6, 2025

rebased to 1.18

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Sorry for the very long delay -- here's some initial feedback. Needs a bunch of work.

Please squash all of your commits and avoid making changes to the existing structure of the code as much as possible (or isolate such changes in separate commits).

src/iter.rs Outdated
@@ -57,11 +57,97 @@ where
}
}

#[derive(Debug)]
struct RingBuf<const SIZE: usize = 10> {
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem like making SIZE a generic const is helpful since we're not actually using that in this project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that was to make playing with different sizes easier for me, i can take it out in the final commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think i will be keeping this, as i want to avoid magic numbers.

@djugei
Copy link
Contributor Author

djugei commented Jul 10, 2025

Please squash all of your commits

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 djugei force-pushed the heuristic branch 2 times, most recently from bb9ce0d to b408abb Compare July 13, 2025 07:32
djugei added 2 commits July 13, 2025 09:38
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.
@djugei
Copy link
Contributor Author

djugei commented Jul 13, 2025

I have done all(-1) of the requested changes, also split out the bug fix into a separate commit while squashing all other changes.

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