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

Improve cpu/time performance by reusing request goroutines. #781

Closed
cyriltovena opened this issue Jul 18, 2019 · 4 comments · Fixed by #5081
Closed

Improve cpu/time performance by reusing request goroutines. #781

cyriltovena opened this issue Jul 18, 2019 · 4 comments · Fixed by #5081
Labels
component/loki keepalive An issue or PR that will be kept alive and never marked as stale. type/enhancement Something existing could be improved

Comments

@cyriltovena
Copy link
Contributor

cyriltovena commented Jul 18, 2019

During benchmark I realized that 35% of cpu time is spent increasing goroutines stack size see flamegraph below. The remaining 65% is gzip decompression.

image

This is happening because Loki query code is very deep in term of function calls, and because each request creates a new goroutine with an initial stacksize of 2KB, which means each goroutines has to growth by factor 2 (this is how go runtime works) then copy the previous stack in the new one. Once a request is done, the goroutine is thrown away and next request will create a new one, that will also have to grow

A more detailed explanation can be found here : https://eng.uber.com/optimizing-m3/

My suggestion here is to create a worker/goroutine pool to use for each query request so that we can recycle goroutines instead of throwing them away.

WDYT ? This seems to increase the speed by 35%. I already get rid of all possible recursions we used to have.

/cc @gouthamve @tomwilkie

@sh0rez sh0rez added the type/enhancement Something existing could be improved label Jul 22, 2019
@stale
Copy link

stale bot commented Sep 3, 2019

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Sep 3, 2019
@cyriltovena
Copy link
Contributor Author

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Sep 3, 2019
@cyriltovena cyriltovena added the keepalive An issue or PR that will be kept alive and never marked as stale. label Sep 3, 2019
@SasSwart
Copy link
Contributor

SasSwart commented Jan 3, 2022

Hey all,

I would like to contribute a change to introduce a memory ballast as discussed above.
As per your CONTRIBUTING.md, I'd like to discuss it here before I begin.

There is a change proposal here that might alleviate the need for this memory ballast, but it is unclear if/when the proposed GC changes would make it into the language.

I think it would therefore still be useful to implement a ballast.
I see that this issue has been marked as stale.
May I begin work on this and provide a PR when done?

@jeschkies
Copy link
Contributor

@SasSwart no one will stop you 🙂 I need to read up on the issue to make a better judgement though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/loki keepalive An issue or PR that will be kept alive and never marked as stale. type/enhancement Something existing could be improved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants