Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mroth opened this issue Jul 30, 2023 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@mroth
Copy link

mroth commented Jul 30, 2023

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 b7a2d41 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 #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.

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 #39214

@rittneje
Copy link

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

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 31, 2023
@dr2chase
Copy link
Contributor

@rsc

mroth added a commit to mroth/go that referenced this issue Aug 1, 2023
This CL is designed to support a larger discussion which is taking place
in golang#61658.

Since golang#39214 (accepted proposal), testing attempts to output the CPU
brand string to provide more context to shared benchmarks.

sysinfo currently relies on internal/cpu for hardware based CPU brand
string detection, which only works on Intel x86 (386 and amd64).

This CL adds a trivial system based fallback implementation for darwin
utilizing syscall, which returns proper brand string on now very common
arm64 darwin devices.

It also sets up the basic structure to support additional platform based
system implementations (for example, adding /proc/cpuinfo system
implementation for Linux), which could come in future CLs.

Updates golang#61658
mroth added a commit to mroth/go that referenced this issue Aug 1, 2023
This CL is designed to support a larger discussion which is taking place
in golang#61658.

Since golang#39214 (accepted proposal), testing attempts to output the CPU
brand string to provide more context to shared benchmarks.

sysinfo currently relies on internal/cpu for hardware based CPU brand
string detection, which only works on Intel x86 (386 and amd64).

This CL adds a trivial system based fallback implementation for darwin
utilizing syscall, which returns proper brand string on now very common
arm64 darwin devices.

It also sets up the basic structure to support additional platform based
system implementations (for example, adding /proc/cpuinfo system
implementation for Linux), which could come in future CLs.

Updates golang#61658
@gopherbot
Copy link

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

@mroth
Copy link
Author

mroth commented Aug 1, 2023

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
Copy link
Author

mroth commented Sep 5, 2023

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.

@mroth mroth changed the title internal/sysinfo: fallbacks for CPU brandname detection on linux+darwin on non-Intel CPU internal/sysinfo: fallbacks for CPU brandname detection on darwin on non-Intel CPU Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants