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.

Sign up for GitHub

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

Jump to bottom

Add optimized ReadAll function #229

Merged
merged 10 commits into from
Oct 2, 2024
Merged

Add optimized ReadAll function #229

merged 10 commits into from
Oct 2, 2024

Conversation

mauri870
Copy link
Member

@mauri870 mauri870 commented Sep 20, 2024 •
edited
Loading

What does this PR do?

This adds an optimized io.ReadAll that uses a bytes.Buffer + io.Copy
internally, improving the buffer growth ratio over the runtime append
and taking advange of io.WriterTo when available.

The only scenario where this optimized version is slower than io.ReadAll is
when reading < 512 bytes of a reader that does not implement io.WriterTo.

For now I only updated tests to use the new function as updating the code
requires more careful consideration.

While at it, migrate from the deprecated ioutil package to io.

goos: linux
goarch: amd64
pkg: github.com/elastic/elastic-agent-libs/iobuf
cpu: Intel(R) Xeon(R) CPU E5-2697A v4 @ 2.60GHz
BenchmarkReadAll/size_32B/io.ReadAll/WriterTo-32         	 7479658	       162.4 ns/op	     512 B/op	       1 allocs/op
BenchmarkReadAll/size_32B/io.ReadAll/Reader-32           	 7000012	       169.3 ns/op	     512 B/op	       1 allocs/op
BenchmarkReadAll/size_32B/ReadAll/WriterTo-32            	10630838	       112.3 ns/op	     112 B/op	       2 allocs/op
BenchmarkReadAll/size_32B/ReadAll/Reader-32              	 2111246	       567.3 ns/op	    1584 B/op	       3 allocs/op
BenchmarkReadAll/size_64B/io.ReadAll/WriterTo-32         	 7154652	       163.2 ns/op	     512 B/op	       1 allocs/op
BenchmarkReadAll/size_64B/io.ReadAll/Reader-32           	 7223264	       166.1 ns/op	     512 B/op	       1 allocs/op
BenchmarkReadAll/size_64B/ReadAll/WriterTo-32            	10400562	       113.5 ns/op	     112 B/op	       2 allocs/op
BenchmarkReadAll/size_64B/ReadAll/Reader-32              	 2129949	       558.7 ns/op	    1584 B/op	       3 allocs/op
BenchmarkReadAll/size_512B/io.ReadAll/WriterTo-32        	 2843871	       419.1 ns/op	    1408 B/op	       2 allocs/op
BenchmarkReadAll/size_512B/io.ReadAll/Reader-32          	 2871580	       413.1 ns/op	    1408 B/op	       2 allocs/op
BenchmarkReadAll/size_512B/ReadAll/WriterTo-32           	 4976233	       241.5 ns/op	     560 B/op	       2 allocs/op
BenchmarkReadAll/size_512B/ReadAll/Reader-32             	 2183186	       552.9 ns/op	    1584 B/op	       3 allocs/op
BenchmarkReadAll/size_10KB/io.ReadAll/WriterTo-32        	  142633	      8235 ns/op	   46080 B/op	      10 allocs/op
BenchmarkReadAll/size_10KB/io.ReadAll/Reader-32          	  148326	      8229 ns/op	   46080 B/op	      10 allocs/op
BenchmarkReadAll/size_10KB/ReadAll/WriterTo-32           	  574903	      2210 ns/op	   10288 B/op	       2 allocs/op
BenchmarkReadAll/size_10KB/ReadAll/Reader-32             	  147210	      7995 ns/op	   32304 B/op	       7 allocs/op
BenchmarkReadAll/size_100KB/io.ReadAll/WriterTo-32       	   13171	     90853 ns/op	  514304 B/op	      18 allocs/op
BenchmarkReadAll/size_100KB/io.ReadAll/Reader-32         	   12892	     91787 ns/op	  514304 B/op	      18 allocs/op
BenchmarkReadAll/size_100KB/ReadAll/WriterTo-32          	   51472	     22420 ns/op	  106544 B/op	       2 allocs/op
BenchmarkReadAll/size_100KB/ReadAll/Reader-32            	   21568	     55070 ns/op	  261680 B/op	      10 allocs/op
BenchmarkReadAll/size_1MB/io.ReadAll/WriterTo-32         	    1220	    983276 ns/op	 5241098 B/op	      27 allocs/op
BenchmarkReadAll/size_1MB/io.ReadAll/Reader-32           	    1089	    990818 ns/op	 5241100 B/op	      27 allocs/op
BenchmarkReadAll/size_1MB/ReadAll/WriterTo-32            	    4153	    294507 ns/op	 1048627 B/op	       2 allocs/op
BenchmarkReadAll/size_1MB/ReadAll/Reader-32              	    1195	    944781 ns/op	 4193852 B/op	      14 allocs/op

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

Related issues

@mauri870 mauri870 added enhancement New feature or request Team:Elastic-Agent Label for the Agent team labels Sep 20, 2024
@mauri870 mauri870 requested a review from a team as a code owner September 20, 2024 12:11
@mauri870 mauri870 requested review from leehinman and khushijain21 and removed request for a team September 20, 2024 12:11
@mauri870 mauri870 changed the title refactor: add optimized ReadAll function Add optimized ReadAll function Sep 20, 2024
@mauri870 mauri870 marked this pull request as draft September 20, 2024 12:12
@mauri870 mauri870 force-pushed the bytes-buffer-readall branch 2 times, most recently from 0b21e08 to e3c4851 Compare September 20, 2024 12:34
mauri870
mauri870 commented Sep 20, 2024
View reviewed changes
iobuf/iobuf.go
// specific language governing permissions and limitations
// under the License.

package iobuf
Copy link
Member Author

@mauri870 mauri870 Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Honestly, I'm terrible at naming things. I'm accepting suggestions for a package name or a better place to put this function.

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Sep 20, 2024
@mauri870 mauri870 marked this pull request as ready for review September 20, 2024 14:44
@mauri870 mauri870 requested review from rdner and removed request for khushijain21 September 20, 2024 16:04
@mauri870 mauri870 mentioned this pull request Oct 1, 2024
Closed
rdner
rdner approved these changes Oct 1, 2024
View reviewed changes
Copy link
Member

@rdner rdner left a comment

Choose a reason for hiding this comment

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

Since we're already fixing linter issues, would be nice to replace how we create temporary directories in tests. Up to you to decide.

I'm fine with the current state of the PR too, given io.ReadAll was left there on purpose.

api/server_test.go Outdated Show resolved Hide resolved
api/server_test.go Outdated Show resolved Hide resolved
file/fileinfo_test.go Outdated Show resolved Hide resolved
file/helper_test.go Outdated Show resolved Hide resolved
keystore/file_keystore_test.go Outdated Show resolved Hide resolved
loader/config_test.go Outdated Show resolved Hide resolved
loader/discover_test.go Outdated Show resolved Hide resolved
keystore/file_keystore.go Show resolved Hide resolved
keystore/file_keystore.go Show resolved Hide resolved
loader/config.go Show resolved Hide resolved
@mauri870
Add optimized ReadAll function
9fde46e 
This adds an optimized io.ReadAll that uses a bytes.Buffer + io.Copy
internally, improving the buffer growth ratio over the runtime append
and taking advange of io.WriterTo when available.

The only scenario where this optimizes version is slower than io.ReadAll is
when reading < 512 bytes of a reader that does not implement io.WriterTo.

For now I only updated tests to use the new function as updating the code
requires more careful consideration.

While at it, migrate from the deprecated ioutil.ReadAll to io.ReadAll.

goos: linux
goarch: amd64
pkg: github.com/elastic/elastic-agent-libs/iobuf
cpu: Intel(R) Xeon(R) CPU E5-2697A v4 @ 2.60GHz
BenchmarkReadAll/size_32B/io.ReadAll/WriterTo-32         	 7479658	       162.4 ns/op	     512 B/op	       1 allocs/op
BenchmarkReadAll/size_32B/io.ReadAll/Reader-32           	 7000012	       169.3 ns/op	     512 B/op	       1 allocs/op
BenchmarkReadAll/size_32B/ReadAll/WriterTo-32            	10630838	       112.3 ns/op	     112 B/op	       2 allocs/op
BenchmarkReadAll/size_32B/ReadAll/Reader-32              	 2111246	       567.3 ns/op	    1584 B/op	       3 allocs/op
BenchmarkReadAll/size_64B/io.ReadAll/WriterTo-32         	 7154652	       163.2 ns/op	     512 B/op	       1 allocs/op
BenchmarkReadAll/size_64B/io.ReadAll/Reader-32           	 7223264	       166.1 ns/op	     512 B/op	       1 allocs/op
BenchmarkReadAll/size_64B/ReadAll/WriterTo-32            	10400562	       113.5 ns/op	     112 B/op	       2 allocs/op
BenchmarkReadAll/size_64B/ReadAll/Reader-32              	 2129949	       558.7 ns/op	    1584 B/op	       3 allocs/op
BenchmarkReadAll/size_512B/io.ReadAll/WriterTo-32        	 2843871	       419.1 ns/op	    1408 B/op	       2 allocs/op
BenchmarkReadAll/size_512B/io.ReadAll/Reader-32          	 2871580	       413.1 ns/op	    1408 B/op	       2 allocs/op
BenchmarkReadAll/size_512B/ReadAll/WriterTo-32           	 4976233	       241.5 ns/op	     560 B/op	       2 allocs/op
BenchmarkReadAll/size_512B/ReadAll/Reader-32             	 2183186	       552.9 ns/op	    1584 B/op	       3 allocs/op
BenchmarkReadAll/size_10KB/io.ReadAll/WriterTo-32        	  142633	      8235 ns/op	   46080 B/op	      10 allocs/op
BenchmarkReadAll/size_10KB/io.ReadAll/Reader-32          	  148326	      8229 ns/op	   46080 B/op	      10 allocs/op
BenchmarkReadAll/size_10KB/ReadAll/WriterTo-32           	  574903	      2210 ns/op	   10288 B/op	       2 allocs/op
BenchmarkReadAll/size_10KB/ReadAll/Reader-32             	  147210	      7995 ns/op	   32304 B/op	       7 allocs/op
BenchmarkReadAll/size_100KB/io.ReadAll/WriterTo-32       	   13171	     90853 ns/op	  514304 B/op	      18 allocs/op
BenchmarkReadAll/size_100KB/io.ReadAll/Reader-32         	   12892	     91787 ns/op	  514304 B/op	      18 allocs/op
BenchmarkReadAll/size_100KB/ReadAll/WriterTo-32          	   51472	     22420 ns/op	  106544 B/op	       2 allocs/op
BenchmarkReadAll/size_100KB/ReadAll/Reader-32            	   21568	     55070 ns/op	  261680 B/op	      10 allocs/op
BenchmarkReadAll/size_1MB/io.ReadAll/WriterTo-32         	    1220	    983276 ns/op	 5241098 B/op	      27 allocs/op
BenchmarkReadAll/size_1MB/io.ReadAll/Reader-32           	    1089	    990818 ns/op	 5241100 B/op	      27 allocs/op
BenchmarkReadAll/size_1MB/ReadAll/WriterTo-32            	    4153	    294507 ns/op	 1048627 B/op	       2 allocs/op
BenchmarkReadAll/size_1MB/ReadAll/Reader-32              	    1195	    944781 ns/op	 4193852 B/op	      14 allocs/op
@mauri870
check error returned by Seek
c5876b2
@mauri870
update ReadAll doc
85f0f87
@mauri870
fix linter error in doc
9595f21
@mauri870
nolint for http.Serve
57a455b
@mauri870
fix license headers
3d2be93
@mauri870
fix code duplication
f310b03
@mauri870
use t.TempDir instead of os.MkdirTemp
4d5d1da
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@mauri870 mauri870 merged commit f1a8d99 into elastic:main Oct 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdner rdner rdner approved these changes

@leehinman leehinman leehinman approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request Team:Elastic-Agent Label for the Agent team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mauri870 @elasticmachine @rdner @leehinman @pierrehilbert