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

runtime: "program exceeds 50-thread limit" in test of os package on darwin-arm-mg912baios [1.13 backport] #34712

Closed
gopherbot opened this issue Oct 6, 2019 · 4 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone

Comments

@gopherbot
Copy link

@odeke-em requested issue #32326 to be considered for backport to the next 1.13 minor release.

Some great news, @minux's CL https://go-review.googlesource.com/c/go/+/197938 fixed this issue in that change from entersyscallblock() to entersyscall() so we'll need to backport Minux's CL to both Go1.12 and Go1.13 as per https://groups.google.com/d/msg/golang-dev/UYDTDWHJsC8/a1_4KiKnCwAJ.

@gopherbot please backport this issue to Go1.12 and Go1.13.

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Oct 6, 2019
@gopherbot gopherbot added this to the Go1.13.2 milestone Oct 6, 2019
@toothrot toothrot added the CherryPickApproved Used during the release process for point releases label Oct 9, 2019
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Oct 9, 2019
@toothrot
Copy link
Contributor

toothrot commented Oct 9, 2019

@minux Backporting approved. Please prepare a cherry-pick CL as per https://github.com/golang/go/wiki/MinorReleases.

@gopherbot
Copy link
Author

Change https://golang.org/cl/200103 mentions this issue: [release-branch.go1.13] runtime: fix darwin syscall performance regression

@gopherbot
Copy link
Author

Change https://golang.org/cl/200105 mentions this issue: [release-branch.go1.13] os: re-enable TestPipeThreads on darwin

@gopherbot
Copy link
Author

Closed by merging 81d995d to release-branch.go1.13.

gopherbot pushed a commit that referenced this issue Oct 9, 2019
…ssion

While understanding why syscall.Read is 2x slower on darwin/amd64, I found
out that, contrary to popular belief, the slowdown is not due to the migration
to use libSystem.dylib instead of direct SYSCALLs, i.e., CL 141639 (and #17490),
but due to a subtle change introduced in CL 141639.

Previously, syscall.Read used syscall.Syscall(SYS_READ), whose preamble called
runtime.entersyscall, but after CL 141639, syscall.Read changes to call
runtime.syscall_syscall instead, which in turn calls runtime.entersyscallblock
instead of runtime.entersyscall. And the entire 2x slow down can be attributed
to this change.

I think this is unnecessary as even though syscalls like Read might block, it
does not always block, so there is no need to handoff P proactively for each
Read. Additionally, we have been fine with not handing off P for each Read
prior to Go 1.12, so we probably don't need to change it. This changes restores
the pre-Go 1.12 behavior, where syscall preamble uses runtime.entersyscall,
and we rely on sysmon to take P back from g blocked in syscalls.

Updates #34712

Change-Id: If76e97b5a7040cf1c10380a567c4f5baec3121ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/197938
Run-TryBot: Minux Ma <minux@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit c1635ad)
Reviewed-on: https://go-review.googlesource.com/c/go/+/200103
Run-TryBot: Alexander Rakoczy <alex@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Oct 9, 2019
CL 197938 actually fixes those regression on Darwin as syscalls
are no longer labeled as always blocking and consume a thread.

Fixes #34712

Change-Id: I82c98516c23cd36f762bc5433d7b71ea8939a0ac
Reviewed-on: https://go-review.googlesource.com/c/go/+/199477
Run-TryBot: Minux Ma <minux@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
(cherry picked from commit cfe2320)
Reviewed-on: https://go-review.googlesource.com/c/go/+/200105
Run-TryBot: Alexander Rakoczy <alex@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@katiehockman katiehockman modified the milestones: Go1.13.2, Go1.13.3 Oct 17, 2019
@golang golang locked and limited conversation to collaborators Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

3 participants