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

utils.BytesBufferGet performance #681

Open
howmoxuan opened this issue Mar 10, 2022 · 9 comments
Open

utils.BytesBufferGet performance #681

howmoxuan opened this issue Mar 10, 2022 · 9 comments

Comments

@howmoxuan
Copy link

in utils.BytesBufferGet, the code use bytesBufferChan. May I know what is the usage/improvement of that channel based on?
In my application, it consume much CPU resource. (the pprof of the application is attached below). Can we just remove it? I do a benchmark, without channel is much faster. Any reason we add that channel?

image

@lance6716
Copy link
Collaborator

by git blame you can find this PR #466.

Can you upload the pprof file, or provide a picture of whole flame graph?

@howmoxuan
Copy link
Author

image

@howmoxuan
Copy link
Author

howmoxuan commented Mar 10, 2022

the moment we do pprof, we just catching the binlog position to latest and no handler for it.

@howmoxuan
Copy link
Author

image

@howmoxuan
Copy link
Author

However, i could not understand why using select-case-default in this case will result in high runtime.Lock. still looking at the code (and learning golang as well)

@moredure
Copy link
Contributor

moredure commented Mar 16, 2022

Is there any special reason for collecting buffers into channels with magical length of 10?
Why not to do just plain utilisation of sync.Pool, since it's already handling data more than one GC cycle pooled.
https://github.com/go-mysql-org/go-mysql/pull/682/files.

Also every 11th+ concurrent call to https://gist.github.com/moredure/4e6b7a1dcd7e0f2d857ca605caf53833 ByteSliceGet will make an allocation, so in highly concurrent environments with high number of requests (more than 10 concurrent requests) ByteSliceGet will allocate, due to sync.Pool interface{} allocates in case of []byte, so some bytebuffer implementation be better used.

@lance6716
Copy link
Collaborator

by git blame you can find this PR #466.

@atercattus ptal, now two persons are planning to revert your pr 😄

@atercattus
Copy link
Member

atercattus commented Mar 17, 2022

Hello for all!
Extra pool via chan was made for reduce memory allocations 😅 At the time (2 years ago) it gave very good results.

with magical length of 10

In our use case (~1kk fast requests per CPU core) it was enough: this allowed us to save buffers between GCs.
To make it better, it was necessary to have more use cases.

Nevertheless.
38.95% just looks terrible. I will be glad if you remove this chan and leave only the sync.Pool :)

@moredure
Copy link
Contributor

@howmoxuan , we merged a few changes, can you check performance from master branch?

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

No branches or pull requests

4 participants