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 optimized ReadAll for http responses #183

Merged
merged 1 commit into from Feb 13, 2024

Conversation

rdner
Copy link
Member

@rdner rdner commented Feb 12, 2024

What does this PR do?

  1. When the body content length is known it's much faster to pre-allocated the entire buffer once.
  2. When the length is unknown using bytes.NewBuffer + io.Copy is much faster.

Also, added benchmarks to prove the difference.

Why is it important?

When we start using this function in main products it will significantly improve the performance.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works

Benchmark

goos: darwin
goarch: arm64
pkg: github.com/elastic/elastic-agent-libs/transport/httpcommon
BenchmarkReadAll/size:_100/unknown_length/io.ReadAll-10                       	    1000	      1232 ns/op	     576 B/op	       3 allocs/op
BenchmarkReadAll/size:_100/unknown_length/bytes.Buffer+io.Copy-10         	    1000	      1219 ns/op	     224 B/op	       4 allocs/op

BenchmarkReadAll/size:_100/known_length/io.ReadAll-10                     	    1000	      1275 ns/op	     576 B/op	       3 allocs/op
BenchmarkReadAll/size:_100/known_length/bytes.Buffer+io.Copy-10           	    1000	      1206 ns/op	     176 B/op	       3 allocs/op

BenchmarkReadAll/size:_102400/unknown_length/io.ReadAll-10                	    1000	     60858 ns/op	  514405 B/op	      24 allocs/op
BenchmarkReadAll/size:_102400/unknown_length/bytes.Buffer+io.Copy-10      	    1000	      9833 ns/op	  106640 B/op	       8 allocs/op

BenchmarkReadAll/size:_102400/known_length/io.ReadAll-10                  	    1000	     44423 ns/op	  514401 B/op	      24 allocs/op
BenchmarkReadAll/size:_102400/known_length/bytes.Buffer+io.Copy-10        	    1000	      9229 ns/op	  106592 B/op	       7 allocs/op

BenchmarkReadAll/size:_1048576/unknown_length/io.ReadAll-10               	    1000	    474003 ns/op	 5241202 B/op	      33 allocs/op
BenchmarkReadAll/size:_1048576/unknown_length/bytes.Buffer+io.Copy-10     	    1000	     81912 ns/op	 1048722 B/op	       8 allocs/op

BenchmarkReadAll/size:_1048576/known_length/io.ReadAll-10                 	    1000	    457693 ns/op	 5241196 B/op	      33 allocs/op
BenchmarkReadAll/size:_1048576/known_length/bytes.Buffer+io.Copy-10       	    1000	     80061 ns/op	 1048674 B/op	       7 allocs/op
PASS
ok  	github.com/elastic/elastic-agent-libs/transport/httpcommon	1.543s

Related issues

1. When the body content length is known it's much faster to
pre-allocated the entire buffer once.
2. When the length is unknown using `bytes.NewBuffer` + `io.Copy` is
much faster.

Also, added benchmarks to prove the difference.
@rdner rdner added the enhancement New feature or request label Feb 12, 2024
@rdner rdner self-assigned this Feb 12, 2024
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @rdner

@rdner rdner marked this pull request as ready for review February 12, 2024 15:54
@rdner rdner requested a review from a team as a code owner February 12, 2024 15:54
@rdner rdner requested review from belimawr, fearful-symmetry, alexsapran and cmacknz and removed request for a team February 12, 2024 15:54
@pierrehilbert pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Feb 12, 2024
@rdner rdner enabled auto-merge (squash) February 12, 2024 18:05
@rdner rdner merged commit a275281 into elastic:main Feb 13, 2024
7 checks passed
@rdner rdner deleted the optimised-http-readall branch February 13, 2024 08:25
@amitkanfer
Copy link

❤️

@cmacknz
Copy link
Member

cmacknz commented Feb 14, 2024

golang/go#50774 appears to be the upstream Go issue tracking the inefficiency of io.ReadAll here. Linking it for future reference.

Quoting the explanation of what causes this:

The underlying reason is that ReadAll() uses append to resize its buffer, which goes up by a factor of 1.25 (slightly more in latest Go, but asymptotically the same) each time.

One might measure these alternatives on various axes:

number of bytes allocated and freed
max size of heap required to read a given size
amount of copying while resizing
percentage waste in returned buffer

The current implementation is better on the last one, and much worse on all the others.

append is designed to minimize space wasted at the end of the buffer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants