-
Notifications
You must be signed in to change notification settings - Fork 126
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
feat(plan-generator): improve memory and cpu usage #1684
base: main
Are you sure you want to change the base?
Conversation
…CPU usage in containerized environments
Router image scan passed✅ No security vulnerabilities found in image:
|
…e on each operation
Can you tell anything about the real world improvements? (Memory etc..) |
@StarpTech Good idea, added in description, thanks! |
// when the system is under memory pressure e.g. when GC is not able to free memory fast enough. | ||
// More details: https://tip.golang.org/doc/gc-guide#Memory_limit | ||
mLimit, err := memlimit.SetGoMemLimitWithOpts( | ||
memlimit.WithRatio(0.9), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think there is any case we want to parameterize this, I would assume not since a use can specify GOMEMLIMIT, wondering if they would want to specify a ratio sometimes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep it like that to follow the same pattern we are following in the router main command: https://github.com/wundergraph/cosmo/blob/main/router/cmd/instance.go#L37
// More details: https://tip.golang.org/doc/gc-guide#Memory_limit | ||
mLimit, err := memlimit.SetGoMemLimitWithOpts( | ||
memlimit.WithRatio(0.9), | ||
memlimit.WithProvider(memlimit.FromCgroupHybrid), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment about memlimit.FromCgroupHybrid
logger.Info(fmt.Sprintf(msg, args...)) | ||
})) | ||
if err != nil { | ||
logger.Fatal(fmt.Sprintf("could not set max GOMAXPROCS: %s", err.Error())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use the zap logger semantics by default, without needing to use fmt.Sprintf, eg :-
logger.Fatal("Could not start router", zap.Error(err))
you can do this for other similar places here, and use the correct type, eg :- zap.String
etc
Motivation and Context
A customer is having issues with how much memory the query planner is using. After some debugging we found two things:
This PR is acting on this two issues:
go.uber.org/automaxprocs/maxprocs
andgithub.com/KimMachineGun/automemlimit/memlimit
to recognize when running inside a container and set the right limitsChecklist
Results
Testing with a big configuration (9MB), a concurrency of 20 and 643 queries, with the query-plan deployed with the router 0.189.2, we got the following result:
The results vary a bit on each execution, by they are always arount this values.
With the router build from this PR we got the following result:
So we went from a peak RSS of 2GB to 0.5GB!