-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
libct/cgroups/fs: code cleanups #2498
Conversation
To my surprise, those are not used anywhere in the code. Signed-off-by: Kir Kolyshkin <[email protected]>
Half of controllers' GetStats just return nil, and most of the others ignore ENOENT on files, so it will be cheaper to not check that the path exists in the main GetStats method, offloading that to the controllers. Drop PathExists check from GetStats, add it to those controllers' GetStats where it was missing. Signed-off-by: Kir Kolyshkin <[email protected]>
It does not add any value. Signed-off-by: Kir Kolyshkin <[email protected]>
2da8af0
to
daf30cb
Compare
Instead of iterating over m.paths, iterate over subsystems and look up the path for each. This is faster since a map lookup is faster than iterating over the names in Get. A quick benchmark shows that the new way is 2.5x faster than the old one. Note though that this is not done to make things faster, as savings are negligible, but to make things simpler by removing some code. Signed-off-by: Kir Kolyshkin <[email protected]>
For the reference, here's the benchmark used for the last commit package fs
import (
"testing"
)
func initPaths() map[string]string {
paths := make(map[string]string)
for _, ss := range subsystems {
paths[ss.Name()] = "/sys/fs/cgroup/" + ss.Name() + "/user.slice/user-1000.slice/session-881.scope/xx88"
}
return paths
}
func BenchmarkIterateOverPaths(b *testing.B) {
paths := initPaths()
b.ResetTimer()
for i := 0; i < b.N; i++ {
for name, path := range paths {
ss, err := subsystems.Get(name)
if err == errSubsystemDoesNotExist || path == "" || ss == nil {
continue
}
}
}
}
func BenchmarkIterateOverSubsystems(b *testing.B) {
paths := initPaths()
b.ResetTimer()
for i := 0; i < b.N; i++ {
for _, ss := range subsystems {
path := paths[ss.Name()]
if path == "" {
continue
}
}
}
} |
type subsystem interface { | ||
// Name returns the name of the subsystem. | ||
Name() string | ||
// Returns the stats, as 'stats', corresponding to the cgroup under 'path'. | ||
GetStats(path string, stats *cgroups.Stats) error | ||
// Removes the cgroup represented by 'cgroupData'. |
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.
This could be of use to external clients and maybe later on if/when we factor this out into a cgroups library.
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.
Theoretically, yes.
Practically,
- I don't see it being used by kubernetes;
- It uses
d.path
which is expensive, it requires to parse mountinfo (this is actually the main point of it being removed); - Its API is not in line with e.g.
Set
(which does not used.path
but the path is provided directly).
Also, if needed, it can be trivial to reimplement / re-add, as it doesn't do anything special, so I see no point in keeping it.
this is separated out from #2438 in order to make review easier
This contains some minor cleanups and optimizations, but mostly just code removal.
TODO: add* db45ce7* 44a02d2once #2496 is merged