replace strings.Split(N) for strings.Cut() or alternatives#7631
replace strings.Split(N) for strings.Cut() or alternatives#7631kzys merged 2 commits intocontainerd:mainfrom
Conversation
| RequiredPlugins: []string{"old_plugin"}, | ||
| DisabledPlugins: []string{"old_plugin"}, | ||
| RequiredPlugins: []string{"io.containerd.old_plugin.v1"}, | ||
| DisabledPlugins: []string{"io.containerd.old_plugin.v1"}, |
There was a problem hiding this comment.
The existing test should pass without modification ?
There was a problem hiding this comment.
Ah, yes, I wanted to post a comment about that, but looks like I forgot.
It's not needed indeed to make the set pass; reason I made the change was to make the test values better match reality (as we don't allow the old values to be used).
Happy to revert that change if you prefer not to update them though (let me know)
|
|
||
| stat.Name = parts[1] | ||
| _, err = fmt.Sscanf(parts[0], "%d", &stat.PID) | ||
| stat.Name = name |
There was a problem hiding this comment.
parts[1] -> name here and then parts[0] -> val, something is backwards, was it incorrect previously?
There was a problem hiding this comment.
Good catch. I don't think I changed that one intentionally 🤔🙀
Would be fun though if it unintentionally fixed a bug 😂
I'm not at my computer right now to view it in within context, but will look later (of course feel free to beat me to that)
There was a problem hiding this comment.
Updated 👍
I may have a look at updating this function to the latest one from runc, which received a bunch of fixes and improvements; opencontainers/runc#2696
I can either use it from there, or update the fork we have here (will have a look what works best)
thaJeztah
force-pushed
the
strings_cut
branch
from
54e58c0 to
05fc6fa
Compare
| b := &Config{ | ||
| Root: "new_root", | ||
| RequiredPlugins: []string{"new_plugin1", "new_plugin2"}, | ||
| RequiredPlugins: []string{"io.containerd.new_plugin1.v1", "io.containerd.new_plugin2.v1"}, |
There was a problem hiding this comment.
Hm.. good one; looks like Version was used here to verify the "integer" cases;
containerd/services/server/config/config.go
Lines 322 to 323 in 39259a8
But a config without Version was deprecated, so let me add a test case using OomScore for this.
We should probably look what we want the behaviour to be if the override file does not specify a version, or if the version is different from the original one; i.e., do we want mergeConfig() to produce an error when merging a v2 config with a v1 config (or v3 with v2)?
There was a problem hiding this comment.
Updated; moved these changes to a separate commit as well (to separate it from the strings.Cut changes)
…alues This updates the test to: - Use correctly formatted values for RequiredPlugins and DisabledPlugins (values are expected to have a `io.containerd.` prefix). While not needed for the test to pass (no validation is performed), it's good to have these values in the correct format (in case we want to add validation at this stage). - Set a `Version` for both (as version 1 / no version was deprecated) The `Version` field in this test was used to verify the "integer override" behavior; setting "Version: 2" for both would no longer cover that case. As there are only 2 integer fields in the config (Version and OOMScore) and OOMScore was already used in the test, I added separate test-cases for that. Looking at the test, we should consider what we want the behaviour to be if the override file does not specify a version (implicitly: version 1), or if the version is different from the original one; do we want mergeConfig() to produce an error when merging a v2 config with a v1 config (or v3 with v2)? Signed-off-by: Sebastiaan van Stijn <[email protected]>
Go 1.18 and up now provides a strings.Cut() which is better suited for
splitting key/value pairs (and similar constructs), and performs better:
```go
func BenchmarkSplit(b *testing.B) {
b.ReportAllocs()
data := []string{"12hello=world", "12hello=", "12=hello", "12hello"}
for i := 0; i < b.N; i++ {
for _, s := range data {
_ = strings.SplitN(s, "=", 2)[0]
}
}
}
func BenchmarkCut(b *testing.B) {
b.ReportAllocs()
data := []string{"12hello=world", "12hello=", "12=hello", "12hello"}
for i := 0; i < b.N; i++ {
for _, s := range data {
_, _, _ = strings.Cut(s, "=")
}
}
}
```
BenchmarkSplit
BenchmarkSplit-10 8244206 128.0 ns/op 128 B/op 4 allocs/op
BenchmarkCut
BenchmarkCut-10 54411998 21.80 ns/op 0 B/op 0 allocs/op
While looking at occurrences of `strings.Split()`, I also updated some for alternatives,
or added some constraints; for cases where an specific number of items is expected, I used `strings.SplitN()`
with a suitable limit. This prevents (theoretical) unlimited splits.
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
strings_cut
branch
from
05fc6fa to
eaedadb
Compare
5 participants
Go 1.18 and up now provides a strings.Cut() which is better suited for splitting key/value pairs (and similar constructs), and performs better:
While looking at occurrences of
strings.Split(), I also updated some for alternatives, or added some constraints; for cases where an specific number of items is expected, I usedstrings.SplitN()with a suitable limit. This prevents (theoretical) unlimited splits.