libct/specconv: reduce init allocations, use global maps, refactor #3281
Conversation
kolyshkin
force-pushed
the
specconv-maps
branch
2 times, most recently
from
bd3813a to
6a1b7e7
Compare
Eliminate some of these allocations when starting runc: > init github.com/opencontainers/runc/libcontainer/specconv @10 ms, 0.11 ms clock, 5408 bytes, 70 allocs Most of this (4K) is the two regexes, which are left intact for now. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These two maps are the same, except that mountPropagationMapping has an extra element with key of "" and value of 0. Since the code already checks for f != 0, this extra element is not a problem. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This makes the repeated calls to parseMountOptions faster, and decreases the amount of garbage to collect. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
parseMountOption already returns way too many values, making the code kind of hard to read. Since all of the return values are used as is to populate the fields of configs.Mount, let's change it to return (semi-)populated *configs.Mount instead. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 1cd71df added isSecSuffix, but the same thing can be done easily without a regex. This is faster and saves some init time and memory. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Also, add a simple test and a benchmark (just out of sheer curiosity). Benchmark results: name old time/op new time/op delta IsValidName-4 540ns ± 3% 45ns ± 1% -91.76% (p=0.008 n=5+5) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
specconv-maps
branch
from
6a1b7e7 to
029b73c
Compare
|
@thaJeztah @mrunalp PTAL |
| name = trimName + "USec" | ||
| value, err = convertSecToUSec(value) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Annotation %s=%s value parse error: %w", k, v, err) |
There was a problem hiding this comment.
Not something new, so probably better to fix separately, but this should've been lowercase
There was a problem hiding this comment.
Are you aware of any linter for such things? Would be nice to have
There was a problem hiding this comment.
I expect the revive linter to be warning about it; 2cca114
| var isValidName = regexp.MustCompile(`^[a-zA-Z]{3,}$`).MatchString | ||
| // isValidName checks if systemd property name is valid. It should consists | ||
| // of latin letters only, and have least 3 of them. | ||
| func isValidName(s string) bool { |
There was a problem hiding this comment.
Related to my other comment (no need to fix in this PR); perhaps would be nice to have this return an error with the reason for it being invalid (too short or invalid character)
There was a problem hiding this comment.
Makes sense; opened #3285
| @@ -256,6 +267,8 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) { | |||
| config.Cgroups = c | |||
| // set linux-specific config | |||
| if spec.Linux != nil { | |||
| initMaps() | |||
There was a problem hiding this comment.
I like the improvement that's brought by this, but (as a follow-up) perhaps we should consider having utilities / accessor functions for these variables, so that the initialisation happens more 'organically' (reducing the cognitive overload of having to thing about first calling initMap() whenever we need to use one of the variables.
In this case it could be a function to check if foo exists in mountPropagationMapping
There was a problem hiding this comment.
Do you mean something like
func getNamespaceMapping() map[specs.LinuxNamespaceType]configs.NamespaceType {
initMaps()
return namespaceMapping
}
// ... and so on for every other map we lazy init.
....
nsMap := getNamespaceMapping()
To me it looks like a bloat, since the nsMap := getNamespaceMapping() is just a way to sugar-coat initMaps().
A runtime assertion would be nice. If only accessing a nil map would panic....
There was a problem hiding this comment.
I can't think of anything better than
if namespaceMapping == nil {
panic("uninitialied map") // we have a bug in the code
}but it's still inferior to
initMaps()There was a problem hiding this comment.
Some kind of a linter to which we can add a rule (in the source code) like "X should be called before using Y" would also be nice to have.
There was a problem hiding this comment.
Ideally, a lazy way to initialize variables. IOW same as this PR does, but automatically (perhaps via an annotation).
There was a problem hiding this comment.
Someone worked on something like this -- alas it does not look good without generics. https://github.com/zetamatta/go-lazy
I guess we have to postpone it till Go 2.0 ))))
This is a set of improvements for libct/specconv. See individual commits for details.
The refactoring should help #3227