libct/cgroups/fs: code cleanups#2498
Merged
Merged
Conversation
This was referenced Jul 3, 2020
To my surprise, those are not used anywhere in the code. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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 <kolyshkin@gmail.com>
It does not add any value. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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 <kolyshkin@gmail.com>
AkihiroSuda
approved these changes
Jul 7, 2020
Contributor
Author
|
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
}
}
}
} |
mrunalp
reviewed
Jul 9, 2020
| 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'. |
Contributor
There was a problem hiding this comment.
This could be of use to external clients and maybe later on if/when we factor this out into a cgroups library.
Contributor
Author
There was a problem hiding this comment.
Theoretically, yes.
Practically,
- I don't see it being used by kubernetes;
- It uses
d.pathwhich 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.pathbut 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.
mrunalp
approved these changes
Jul 9, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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