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

Add download_chunk_size property to HTTPRequest. #33862

Merged
merged 1 commit into from
Nov 25, 2019

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Nov 24, 2019

This allows setting the read_chunk_size of the internal HTTPClient.
This is important to reduce the allocation overhead and number of file
writes when downloading large files, allowing for better download speed.

Fixes #32807 (the user will need to raise the download_chunk_size to a higher value. 64KiB is enough).

extends Control

var done := false
var last_bytes := 0
var time := 0.0 # timer that is restarted ~every second
var http

func _ready():
	http = HTTPRequest.new()
	# Sets a 64KiB chunk size
	http.download_chunk_size = pow(2,16)
	add_child(http)
	http.use_threads = true
	http.download_file = "100MB.bin"
	http.connect("request_completed", self, "_on_file_request_completed")

	# 100MB test file
	http.request("https://speed.hetzner.de/100MB.bin")

func _process(delta):
	if not done and time >= 1:
		var speed_in_bytes = http.get_downloaded_bytes() - last_bytes
		var speed_in_mega_bytes = speed_in_bytes / 1000000.0
		
		print(speed_in_mega_bytes, " MB/s")
		
		last_bytes = http.get_downloaded_bytes()
		time = 0
	time += delta

func _on_file_request_completed(result, response_code, headers, body):
	done = true
	print("File downloaded")

In general, and like in other cases in Godot networking, we are doing some extra buffering, which could be cleaned up in the future.

This allows setting the `read_chunk_size` of the internal HTTPClient.
This is important to reduce the allocation overhead and number of file
writes when downloading large files, allowing for better download speed.
@Faless Faless added this to the 3.2 milestone Nov 24, 2019
@Faless Faless requested a review from a team as a code owner November 24, 2019 18:39
@Faless
Copy link
Collaborator Author

Faless commented Nov 24, 2019

Note I think we should raise the default value to something higher, 32-64KiB, to better cover the download file use case by default. In any case, we should do that for editor-related HTTPRequest where download speed is quite important.

@akien-mga
Copy link
Member

Note I think we should raise the default value to something higher, 32-64KiB, to better cover the download file use case by default.

Sounds good to me. Are there any drawbacks to using a value like 64 KiB by default?

@Faless
Copy link
Collaborator Author

Faless commented Nov 24, 2019

Are there any drawbacks to using a value like 64 KiB by default?

Well, there might be some memory overhead, especially when reading in non-blocking mode from slow servers (or servers that sends little data at a time).

But in many other case (like the referenced issue) it would mean a much lower number of allocations.

In general, HTTPClient and HTTPRequest likely need some refactoring to use pre-allocated buffers instead of allocating memory every time (unless when strictly talking to GDScript). But that's 4.0 material.

@akien-mga akien-mga merged commit 967cc2c into godotengine:master Nov 25, 2019
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

I think a default of 64 KiB would make sense. Or 32 KiB if you think 64 KiB is too high as all-rounder value (knowing that most users will use the default). Can you make a follow-up PR?

@neqs20
Copy link

neqs20 commented Dec 8, 2019

Now it works like a charm, Thanks Faless.
I think download_chunk_size should be by default 32 KiB because downloaded files are often small sizes.
In this case there's no need to occupy additional memory for such a file. If a user wants to download larger files, he just needs to increase the value. But the user has to read docs in a first place...

Calinou added a commit to Calinou/godot that referenced this pull request Nov 7, 2020
This improves download speeds at the cost of increased memory usage.

This change also effects HTTPRequest automatically.

See godotengine#32807 and godotengine#33862.
akien-mga pushed a commit to akien-mga/godot that referenced this pull request Nov 11, 2020
This improves download speeds at the cost of increased memory usage.

This change also effects HTTPRequest automatically.

See godotengine#32807 and godotengine#33862.

(cherry picked from commit 1335709)
GryphonClaw pushed a commit to GryphonClaw/godot that referenced this pull request Nov 19, 2020
This improves download speeds at the cost of increased memory usage.

This change also effects HTTPRequest automatically.

See godotengine#32807 and godotengine#33862.
HEAVYPOLY pushed a commit to HEAVYPOLY/godot that referenced this pull request Dec 14, 2020
This improves download speeds at the cost of increased memory usage.

This change also effects HTTPRequest automatically.

See godotengine#32807 and godotengine#33862.

(cherry picked from commit 1335709)
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.

Slow download speed of HTTPRequest node
3 participants