golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.05k stars 17.54k forks source link

internal/sysinfo: fallbacks for CPU brandname detection on darwin on non-Intel CPU #61658

Open mroth opened 1 year ago

mroth commented 1 year ago

What version of Go are you using (go version)?

$ go version
go version go1.20.6 darwin/arm64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/mroth/Library/Caches/go-build"
GOENV="/Users/mroth/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/mroth/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/mroth/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.20.6/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.20.6/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.6"
GCCGO="gccgo"
AR="ar"
CC="cc"
CXX="c++"
CGO_ENABLED="1"
GOMOD="/Users/mroth/src/xpe/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/q5/cp5w3_v97rg7n9w19dtntfsr0000gn/T/go-build1498865535=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Run any benchmarks (e.g go test -bench=.) on a darwin/arm64 or linux/arm64 machine.

What did you expect to see?

A prelude to the benchmark output containing a cpu line, ala:

$ go test -bench=.*
goos: darwin
goarch: amd64
pkg: strconv
cpu: Intel(R) Core(TM) i7-3520M CPU @ 2.90GHz
BenchmarkAtof64Decimal-4            24431032            46.8 ns/op

Since https://github.com/golang/go/commit/b7a2d413a3f710f14accedf185c93bfb63d24fd0 benchmark test output contains cpu field designed to aide in the recognizable and shareable usage of the output; however the current implementation only works on Intel x86.

What did you see instead?

Prelude which contains goos goaarch and pkg, but no cpu field.

goos: darwin
goarch: arm64
pkg: github.com/mroth/foo
BenchmarkFoo-12        131043              7868 ns/op
PASS
ok      github.com/mroth/foo    1.229s

I see this behavior (as expected) on both darwin/arm64 and linux/arm64 hardware I have access to test on.


Proposed solution discussion (and sample code)

(Opened as a "bug" issue as this change would not affect any exposed APIs, and thus is not a "proposal".)

Reasoning

The initial discussion around adding this information was in https://github.com/golang/go/issues/39214. As arm64 hardware becomes increasingly common (notably, all consumer machines sold by Apple today), we have more and more developers writing Go using this as their development environment. Having the CPU "brand name" would aide in the context understanding of benchmark output shared by these developers.

Implementation

The existing internal/sysinfo implementation is a sync.Once cached call to internal/cpu.CPU.Name(), which is able to pull the brand string directly from the hardware on OS on Intel x86 chipsets. There is unfortunately apparently no reliable CPU-based way to do the same on arm64.

There exists an existing TODO comment line in this package suggesting fallback to platform specific implementations may be acceptable:

TODO(martisch): use /proc/cpuinfo and /sys/devices/system/cpu/ on Linux as fallback.

:star: I have put together a functional sample module at https://github.com/mroth/xsysinfo that shows what these modifications could look like, with support for Linux and Darwin platforms. I would like to open a CL but would like to first align on what is the ideal path is prior to doing so.

Darwin implementation (Syscall)

The Darwin solution was quite simple to implement, as it is a simple syscall to get machdep.cpu.brand_string. As example, on my test hardware, this returns Apple M2 Pro. This implementation should function on both arm64 and older amd64 darwin devices (though on the amd64 variants the cpu-based check will succeed, so this fallback should never be executed).

Linux implementation (ProcFS)

The Linux usage of /proc/cpuinfo does feel a bit brittle as it relies on string parsing, but is perhaps acceptable given this information is not exposed in the go API anywhere, and is only used for developer tooling UX convenience.

In the future, the linux implementation may want to be extended to look for /proc/device-tree/model which seems to be in usage on embedded hardware (e.g. Raspberry Pis which may be being used for local development, etc.)

Windows implementation (future)

I did not create one today, but I believe it should be possible in the future if ARM hardware becomes common on Windows.

Risks

The error case simply elides the cpu field output, which is already the status quo today.

There is a risk the Linux implementation could become increasingly complex in the future if many edge cases are added, adding a maintenance burden. (If this is deemed too risky in discussion here, I would propose I open my CL for review with the darwin implementation only for now, which should remain relatively stable.)


cc @martisch who wrote https://github.com/golang/go/issues/39214

rittneje commented 1 year ago

Note that /proc/cpuinfo does not contain a "model name" entry when running a linux/arm64 container on Apple M1.

dr2chase commented 1 year ago

@rsc

gopherbot commented 1 year ago

Change https://go.dev/cl/515055 mentions this issue: internal/sysinfo: fallback to syscall for CPU detection on darwin

mroth commented 1 year ago

I've added a first CL with the Darwin implementation only to aide in the discussion (linked by gopherbot above).

With this change, benchmark output adds cpu name properly on arm64 Darwin machines:

$ go test -bench=.
goos: darwin
goarch: arm64
pkg: strconv
cpu: Apple M2 Pro
BenchmarkAtof64Decimal-12       57259666            20.98 ns/op

(A sample Linux implementation still exists in the aforementioned https://github.com/mroth/xsysinfo repo, but I feel like there may be a bit more to discuss about the merits of the /proc/cpuinfo approach before adding that as a follow on CL.)

mroth commented 1 year ago

Support for /proc/cpuinfo on Linux was recently added in https://go-review.googlesource.com/c/go/+/508735, so my associated CL adding Darwin CPU detection support for non-Intel CPUs has been rebased to incorporate that structure: https://go-review.googlesource.com/c/go/+/515055 (and should be ready for initial review).

This issue now only tracks adding Darwin support.