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.

Sign up for GitHub

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

Jump to bottom

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
Closed

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

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

Comments

@gopherbot
Copy link
Contributor

@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 mentioned this issue Oct 6, 2019
Closed
@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.

This was referenced Oct 9, 2019
Closed
Closed
Closed
@gopherbot
Copy link
Contributor Author

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

@gopherbot
Copy link
Contributor Author

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

@gopherbot
Copy link
Contributor Author

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

gopherbot pushed a commit that referenced this issue Oct 9, 2019
@minux @toothrot
[release-branch.go1.13] runtime: fix darwin syscall performance regre…
81d995d 
…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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Keith Randall <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
(cherry picked from commit c1635ad)
Reviewed-on: https://go-review.googlesource.com/c/go/+/200103
Run-TryBot: Alexander Rakoczy <[email protected]>
Reviewed-by: Cherry Zhang <[email protected]>
gopherbot pushed a commit that referenced this issue Oct 9, 2019
@minux @toothrot
[release-branch.go1.13] os: re-enable TestPipeThreads on darwin
49073c5 
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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Tobias Klauser <[email protected]>
(cherry picked from commit cfe2320)
Reviewed-on: https://go-review.googlesource.com/c/go/+/200105
Run-TryBot: Alexander Rakoczy <[email protected]>
Reviewed-by: Bryan C. Mills <[email protected]>
Reviewed-by: Cherry Zhang <[email protected]>
@katiehockman katiehockman modified the milestones: Go1.13.2, Go1.13.3 Oct 17, 2019
@rsc rsc mentioned this issue Nov 21, 2019
Open
@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.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.13.3
Development

No branches or pull requests
3 participants
@toothrot @gopherbot @katiehockman