pkg/sysinfo: Deprecate NumCPU#49241
Conversation
vvoland
added
impact/changelog
impact/deprecation
kind/refactor
| // Returns bit count of 1, used by NumCPU | ||
| func popcnt(x uint64) (n byte) { | ||
| x -= (x >> 1) & 0x5555555555555555 | ||
| x = (x>>2)&0x3333333333333333 + x&0x3333333333333333 | ||
| x += x >> 4 | ||
| x &= 0x0f0f0f0f0f0f0f0f | ||
| x *= 0x0101010101010101 | ||
| return byte(x >> 56) | ||
| } | ||
|
|
||
| func numCPU() int { |
There was a problem hiding this comment.
Was the Windows implementation also updated in go's stdlib?
There was a problem hiding this comment.
Yes
The only difference is that it used different (possibly less efficient?) popcnt implementation.
It was updated in commit https://cs.opensource.google/go/go/+/6410e67a1eb38df3cc72cef818ed392bea907251
There was a problem hiding this comment.
Looks like it was in go1.6 already!? golang/go@6410e67
| // NumCPU returns the number of CPUs. It's the equivalent of [runtime.NumCPU]. | ||
| // Deprecated: Use [runtime.NumCPU] instead. It will be removed in the next release. | ||
| var NumCPU = runtime.NumCPU() |
There was a problem hiding this comment.
This needs an empty line before the Deprecated comment, otherwise it's not detected as "Deprecated".
It's also slightly better to create a wrapper function here; both because we don't want any code from overriding the exported alias, but also because otherwise it shows up under variables on pkg.go.dev, which can be confusing.
| // NumCPU returns the number of CPUs. It's the equivalent of [runtime.NumCPU]. | |
| // Deprecated: Use [runtime.NumCPU] instead. It will be removed in the next release. | |
| var NumCPU = runtime.NumCPU() | |
| // NumCPU returns the number of CPUs. It's the equivalent of [runtime.NumCPU]. | |
| // | |
| // Deprecated: Use [runtime.NumCPU] instead. It will be removed in the next release. | |
| func NumCPU() int { | |
| return runtime.NumCPU() | |
| } |
There was a problem hiding this comment.
Ah, good catch, let me update
There was a problem hiding this comment.
I always forget about the required newline before Deprecated 🙈
Sounds like we should teach our linter to detect that 😅
There was a problem hiding this comment.
Yes, actually surprised there's no linter picking on those currently!
for the "wrapper vs direct alias"; both options are OK if things get too complicated (sometimes the wrapper means we now have to import other packages, which may not be worth it in some cases), but in this case it was trivial to change.
Deprecate in favor of `runtime.NumCPU` as the behavior is the same now. Signed-off-by: Paweł Gronowski <[email protected]>
vvoland
force-pushed
the
runtime-numcpu
branch
from
4559a2e to
3db72b2
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
I'd even be comfortable picking this for 27.5 if we still want to include changes; I don't think there's external consumers of this; at least a quick check shows that podman and nerdctl use a fork;
https://github.com/containers/podman/blob/1a00c92e005a490a6a2389618d9f55465cf73141/cmd/podman/pods/create.go#L13
https://github.com/containers/podman/blob/1a00c92e005a490a6a2389618d9f55465cf73141/cmd/podman/pods/create.go#L215
https://github.com/containerd/nerdctl/blob/f5e58defcf5d2a87a91affe43046be8a5dfe2f90/pkg/infoutil/infoutil_windows.go#L31
https://github.com/containerd/nerdctl/blob/f5e58defcf5d2a87a91affe43046be8a5dfe2f90/pkg/infoutil/infoutil_windows.go#L215C14-L215C22
|
cc @AkihiroSuda FYI; you probably can remove this from your nerdctl fork as well |
| Architecture: platform.Architecture(), | ||
| RegistryConfig: doWithTrace(ctx, "registry.ServiceConfig", daemon.registryService.ServiceConfig), | ||
| NCPU: doWithTrace(ctx, "sysinfo.NumCPU", sysinfo.NumCPU), | ||
| NCPU: doWithTrace(ctx, "runtime.NumCPU", runtime.NumCPU), |
There was a problem hiding this comment.
Nit; we could even consider removing the doWithtrace here, as it would now be tracing go stdlib; not sure if there's much use in that.
2 participants
Deprecate in favor of
runtime.NumCPUas the behavior is the same now.- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)