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

go/token: AddFile induces heavy contention in parallel parsing [freeze exception] #53507

Closed
adonovan opened this issue Jun 22, 2022 · 2 comments
Closed

go/token: AddFile induces heavy contention in parallel parsing [freeze exception] #53507

adonovan opened this issue Jun 22, 2022 · 2 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance release-blocker
Milestone
Go1.19

Comments

@adonovan
Copy link
Member

adonovan commented Jun 22, 2022 •
edited

https://go-review.googlesource.com/c/go/+/411909 reduces two sources of contention in parser-heavy workloads: FileSet.AddFile allocates memory in the middle of an otherwise tiny critical section, and FileSet.Pos uses a mutex to guard a cache, turning read-only operations into exclusive locks. Both are major sources of contention in gopls.

I'd like to include the fix in the next release.

@golang/release @gri @findleyr
@cagedmantis cagedmantis added this to the Go1.19 milestone Jun 23, 2022
@cagedmantis cagedmantis added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 23, 2022
@cagedmantis
Copy link
Contributor

This change doesn't seem to pose a significant risk. The release candidate has not been released yet. This change is approved.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. Performance and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 29, 2022
@gopherbot
Copy link

Change https://go.dev/cl/411909 mentions this issue: go/token: use atomics not Mutex for last file cache

jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@adonovan @jproberts
go/token: use atomics not Mutex for last file cache
e877b49 
Previously, FileSet would cache the last *File found by a lookup,
using a full (exclusive) mutex within FileSet.File, turning a logical
read operation into an update. This was one of the largest sources
of contention in gopls.  This change uses atomic load/store on the
'last' field without a mutex.

Also, in FileSet.AddFile, allocate the File outside the critical
section; all the other operations are typically cheap.

Fixes golang#53507

Change-Id: Ice8641650d8495b25b0428e9b9320837ff2ca7e1
Reviewed-on: https://go-review.googlesource.com/c/go/+/411909
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jun 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance release-blocker
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.19
Development

No branches or pull requests
4 participants
@cagedmantis @dmitshur @adonovan @gopherbot