Skip to content

/ go

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

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 FrozenDueToAge
Milestone
Go1.13.3

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 6, 2019

@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 mentioned this issue Oct 6, 2019
Closed
@gopherbot gopherbot added this to the Go1.13.2 milestone Oct 6, 2019
@toothrot
Copy link
Contributor

@toothrot 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
Author

@gopherbot gopherbot commented Oct 9, 2019

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

@gopherbot gopherbot commented Oct 9, 2019

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

@gopherbot gopherbot commented Oct 9, 2019

Closed by merging 81d995d to release-branch.go1.13.
@gopherbot gopherbot closed this Oct 9, 2019
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 <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
@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 <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
@rsc rsc mentioned this issue Nov 21, 2019
Closed
@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 FrozenDueToAge
Projects
None yet
Milestone
Go1.13.3
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
@toothrot @gopherbot @katiehockman
You can’t perform that action at this time.