Skip to content
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

Increase max read #16946

Merged
merged 2 commits into from
Apr 16, 2020
Merged

Increase max read #16946

merged 2 commits into from
Apr 16, 2020

Conversation

exussum12
Copy link
Contributor

@exussum12 exussum12 commented Aug 30, 2019

8kb is very low, especially given this will be local files. a fix for #16945

@kesselb
Copy link
Contributor

kesselb commented Aug 30, 2019

Thank you 👍

Mind to sign off your commits? https://github.com/nextcloud/server/pull/16946/checks?check_run_id=208487803 for more information.

@rullzer
Copy link
Member

rullzer commented Sep 2, 2019

So I actually tried this and for me it made no difference. As the files are read in 8kb chunks anyways (php internals). did you change other things as well?

@exussum12
Copy link
Contributor Author

I guess thats kinda the point. You shouldn't notice a difference. In my set up this stopped bandwidth by a huge amount.

I only looked at this being an issue because of the huge bandwidth usage on my server. This fixed it for me

@exussum12
Copy link
Contributor Author

exussum12 commented Sep 2, 2019

Screenshot_20190902-182938

See above for bandwidth screenshot. Before and after this change.

One of the last 2peaks was me testing the fix. Other was was downloading a massive file.

Just changed these two lines for this effect

@rullzer
Copy link
Member

rullzer commented Sep 8, 2019

@exussum12 I meant that I still saw 8k reads originating from php 😉 as that is one of the internal constants.

I'll dive a bit deeper.

@exussum12
Copy link
Contributor Author

exussum12 commented Sep 8, 2019

Have you got a link to that source? I did take a look but didn't find it. I imagine its there in the context of IP packets and not disk read bytes as many disks actually have larger block sizes than this also now. (Though this effect will be much less noticeable on a disk itself )

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me otherwise

lib/private/Files/View.php Outdated Show resolved Hide resolved
@rullzer
Copy link
Member

rullzer commented Sep 9, 2019

Have you got a link to that source? I did take a look but didn't find it. I imagine its there in the context of IP packets and not disk read bytes as many disks actually have larger block sizes than this also now. (Though this effect will be much less noticeable on a disk itself )

Mmm I can only trigger it in docker on old php versions.
Then I'm good to go.

@kesselb kesselb added this to the Nextcloud 19 milestone Feb 1, 2020
@kesselb
Copy link
Contributor

kesselb commented Feb 1, 2020

@exussum12 could you change the chunkSize to 512 kB (524288) as requested and rebase the branch with the latest changes from master? Thanks 👍

@exussum12 exussum12 force-pushed the patch-1 branch 2 times, most recently from 2080f87 to 79fa714 Compare February 1, 2020 22:25
@kesselb
Copy link
Contributor

kesselb commented Feb 2, 2020

Thanks @exussum12 👍

CI looks good. I restarted the Job because some timeouts.

@rullzer @icewind1991 🏓

@rullzer rullzer mentioned this pull request Apr 4, 2020
80 tasks
@kesselb
Copy link
Contributor

kesselb commented Apr 5, 2020

Hey @exussum12, mind to do another rebase?

@rullzer rullzer mentioned this pull request Apr 9, 2020
59 tasks
8kb is very low, especially given this will be local files

Signed-off-by: Scott Dutton <scott@exussum.co.uk>
@exussum12
Copy link
Contributor Author

exussum12 commented Apr 12, 2020

rebased @kesselb

@@ -423,7 +423,7 @@ public function readfile($path) {
@ob_end_clean();
$handle = $this->fopen($path, 'rb');
if ($handle) {
$chunkSize = 8192; // 8 kB chunks
$chunkSize = 512000; // 500 kB chunks
Copy link
Member

Choose a reason for hiding this comment

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

should be 524288 as per #16946 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry. I think I rebased from a commit without this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@rullzer rullzer mentioned this pull request Apr 15, 2020
57 tasks
@exussum12
Copy link
Contributor Author

The lint check is failing on lines I've not touched ?

@rullzer rullzer mentioned this pull request Apr 16, 2020
55 tasks
@rullzer
Copy link
Member

rullzer commented Apr 16, 2020

The lint check is failing on lines I've not touched ?

yeah not your fault. new linter

@rullzer rullzer merged commit 79cf13e into nextcloud:master Apr 16, 2020
@welcome
Copy link

welcome bot commented Apr 16, 2020

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@exussum12 exussum12 deleted the patch-1 branch April 18, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants