Skip to content

use BufReader to read archive index files #1530

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

Merged
merged 1 commit into from
Oct 26, 2021

Conversation

syphar
Copy link
Member

@syphar syphar commented Oct 26, 2021

I ran test with an index with 17k files:

Since the average archive-size for docs is just around 600 I think we should merge and deploy this first.
Using mmap made it only slightly faster, which is why I didn't add the additional dependency.

The next optimization is probably not to load the whole index into memory but to answer the find-file requests while streaming the content of the index. But I would like to gather more data on archive-sizes first and benchmark it.

@syphar syphar self-assigned this Oct 26, 2021
@syphar syphar added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Oct 26, 2021
@jyn514
Copy link
Member

jyn514 commented Oct 26, 2021

for the problematic crate ( big crates are slow to use with new archive-storage #1528 ) it still takes ~250ms

What was it before this change?

The next optimization is probably not to load the whole index into memory but to answer the find-file requests while streaming the content of the index. But I would like to gather more data on archive-sizes first and benchmark it.

👍

@@ -28,11 +28,14 @@ pub(crate) struct Index {

impl Index {
pub(crate) fn load(reader: impl io::Read) -> Result<Index> {
Copy link
Member

Choose a reason for hiding this comment

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

Can we take BufReader here instead, to avoid having a BufReader<BufReader>?

Copy link
Member Author

Choose a reason for hiding this comment

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

while changing this I saw an issue with that:
sometimes we are using load with a slice. In this case I also used an unnecessary BufReader.

Also, the BufWriter on save doesn't help, because we're saving into a in-memory buffer anyways...

I updated the code, even less now

@jyn514 jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 26, 2021
@syphar
Copy link
Member Author

syphar commented Oct 26, 2021

for the problematic crate ( big crates are slow to use with new archive-storage #1528 ) it still takes ~250ms

What was it before this change?

roughly the same ratio, so over 20 seconds

@syphar syphar force-pushed the archive-index-quickfix branch from f2d80f8 to 8c76fa2 Compare October 26, 2021 16:26
@syphar syphar added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Oct 26, 2021
@syphar syphar requested a review from jyn514 October 26, 2021 16:26
@syphar syphar changed the title use BufReader and BufWriter to read/write archive index files use BufReader to read archive index files Oct 26, 2021
@syphar syphar force-pushed the archive-index-quickfix branch 2 times, most recently from fcbac39 to c8a1f35 Compare October 26, 2021 16:47
@syphar syphar force-pushed the archive-index-quickfix branch from c8a1f35 to c4cbe31 Compare October 26, 2021 16:49
@jyn514 jyn514 added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 26, 2021
@jyn514 jyn514 merged commit f0f19a6 into rust-lang:master Oct 26, 2021
@syphar syphar deleted the archive-index-quickfix branch October 26, 2021 17:46
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Nov 30, 2021
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