Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Dynamic GC control #1171

Closed
wants to merge 8 commits into from
Closed

Dynamic GC control #1171

wants to merge 8 commits into from

Conversation

deniszh
Copy link

@deniszh deniszh commented Dec 9, 2018

Rationale:
when using Kafka input during startup of the node and initial backfill consumption I noticed 50-100% more memory consumption than during normal work.
I tried to mitigate that using GOGC environment variable - but then I found out that it obviously aggressive GOGC affecting rendering performance - and that increased GC pressure is not needed anymore because memory consumption is already lowered.
So, I implemented GOGC control through config file - gogc-startup variable in [kafka-mdm-in] section controls GOGC for program startup, gogc-ready - for normal work.
I also implemented HTTP API for changing these vars, but that part is probably optional.

@Dieterbe
Copy link
Contributor

this is a very cool idea.
FYI, I have looked a bit closer at the startup procedure and documented it. (#1186)
I thought the other phases (metric persist processing, index loading) could also benefit from the more aggressive gogc value during startup (which made me think again about making it a global option in MT and it could set the gogc-startup value sooner), but they don't seem to exert a high memory allocation workload, so we can leave it specific to the kafka-mdm-in plugin

I notice the api does unsafe (unsynchronized) access to the variables though.
also if we keep the api, it needs to be labeled as experimental.

@Dieterbe
Copy link
Contributor

Dieterbe commented Dec 27, 2018

let me know if you're not able to work on this anymore, i may be able to take over.
also, would you be able to rebase on master so we have a clean history with only your commits, merging master into your branch makes it a bit of a mess.

@deniszh
Copy link
Author

deniszh commented Dec 28, 2018

Hi @Dieterbe ,

Right. Just added simple mutex (single for both calls, not sure if good idea) - or I can add 2 separate ones.
I can squash and rebase ofc if looking good for you.
Or feel free to reuse it in own commit if you have same feature in the work - I don't mind at all.

@Dieterbe
Copy link
Contributor

i realized it's not proper in the kafka mdm input plugin to look at the cluster.MaxPrio setting because maxprio/readyness state is a concern for the cluster only, not input plugins.
note that a MT instance mode in single mode (SingleNodeManager) vs multi mode (MemberlistManager)
both use the same thisNode which is a HTTPNode.
both managers simply act on thisNode upon IsReady() and SetPriority() calls
As such, now I think the updating of GOGC should happen in thisNode, whenever ready state flips.
this has the benefit of also working during other operations (e.g. index loading)

i have some WIP code for this that i can push soon

@Dieterbe
Copy link
Contributor

@deniszh have a look at #1194 please, I think that version is better.

@deniszh
Copy link
Author

deniszh commented Dec 29, 2018

@Dieterbe : yes, that's much cleaner, indeed. You decided not to implement api part? That's fine, just curious.

@Dieterbe
Copy link
Contributor

yes. not a real need for it.

@Dieterbe Dieterbe closed this Jan 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants