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

Jump to bottom

runtime: convert atomic values to runtime/internal/atomic types #53821

Open
prattmic opened this issue Jul 12, 2022 · 83 comments
Open

runtime: convert atomic values to runtime/internal/atomic types #53821

prattmic opened this issue Jul 12, 2022 · 83 comments
Assignees
@prattmic
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Backlog

Comments

@prattmic
Copy link
Member

runtime/internal/atomic added Int32, etc types a while ago, which are slowing getting used on new code. We should also go through existing code and convert bare atomic values to use the atomic types.
@prattmic prattmic added this to the Backlog milestone Jul 12, 2022
@prattmic prattmic self-assigned this Jul 12, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 12, 2022
@gopherbot
Copy link

Change https://go.dev/cl/417784 mentions this issue: runtime: convert gcController.dedicatedMarkWorkersNeeded to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/417782 mentions this issue: runtime: convert gcController.fractionalMarkTime to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/417775 mentions this issue: runtime: convert gcController.heapLive to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/417778 mentions this issue: runtime: convert gcController.maxStackScan to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/417783 mentions this issue: runtime: convert gcController.idleMarkTime to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/417777 mentions this issue: runtime: convert gcController.lastStackScan to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/417781 mentions this issue: runtime: convert gcController.dedicatedMarkTime to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/417780 mentions this issue: runtime: convert gcController.bgScanCredit to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/417776 mentions this issue: runtime: convert gcController.heapScan to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/417779 mentions this issue: runtime: convert gcController.globalsScan to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419444 mentions this issue: runtime: convert schedt.ngsys to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419437 mentions this issue: runtime: convert runningPanicDefers to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419438 mentions this issue: runtime: convert panicking to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419442 mentions this issue: runtime: convert schedt.lastpoll to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419445 mentions this issue: runtime: convert schedt.npidle to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419443 mentions this issue: runtime: convert schedt.pollUntil to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419439 mentions this issue: runtime: convert schedt.goidgen to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419436 mentions this issue: runtime: convert pendingPreemptSignals to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419446 mentions this issue: runtime: convert schedt.nmspinning to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419448 mentions this issue: runtime: convert schedt.sysmonwait to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/419449 mentions this issue: runtime: convert timeHistogram to atomic types

@gopherbot
Copy link

Change https://go.dev/cl/419447 mentions this issue: runtime: convert schedt.gcwaiting to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/420195 mentions this issue: runtime: convert prof.signalLock to atomic type

@gopherbot
Copy link

Change https://go.dev/cl/420196 mentions this issue: runtime: convert prof.hz to atomic type

@prattmic prattmic mentioned this issue Jul 29, 2022
Open
gopherbot pushed a commit that referenced this issue Aug 8, 2022
@prattmic
runtime: convert gcController.heapLive to atomic type
3a9281f 
Atomic operations are used even during STW for consistency.

For #53821.

Change-Id: Ibe7afe5cf893b1288ce24fc96b7691b1f81754ff
Reviewed-on: https://go-review.googlesource.com/c/go/+/417775
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Aug 8, 2022
@prattmic
runtime: convert gcController.heapScan to atomic type
6e9925c 
For #53821.

Change-Id: I64d3f53c89a579d93056906304e4c05fc35cd9b3
Reviewed-on: https://go-review.googlesource.com/c/go/+/417776
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
cuiweixie added a commit to cuiweixie/go that referenced this issue Sep 2, 2022
@cuiweixie
runtime: convert local var started,progress at TestStackGrowth to ato…
b9a8021 
…mic type

For golang#53821

Change-Id: I9c777ff642ea4b70073335279551cea6a2394569
cuiweixie added a commit to cuiweixie/go that referenced this issue Sep 2, 2022
@cuiweixie
runtime: convert local var stop at TestAfterStress to atomic type
c8cacc4 
For golang#53821

Change-Id: I7e86dac34691f7752f68879ff379061f3435cd45
@cuishuang cuishuang mentioned this issue Sep 4, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/428335 mentions this issue: internal: convert calls to atomic type

@cuishuang cuishuang mentioned this issue Sep 4, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/428336 mentions this issue: net/http: convert allowQuerySemicolonsInUse to atomic type

@cuishuang cuishuang mentioned this issue Sep 4, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/428195 mentions this issue: expvar: convert f to atomic type

@cuishuang cuishuang mentioned this issue Sep 4, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/428196 mentions this issue: runtime: convert timer.status to atomic type

gopherbot pushed a commit that referenced this issue Sep 5, 2022
@panjf2000 @mvdan
runtime: convert g.atomicstatus to internal atomic type
3fbcf05 
Note that this changes some unsynchronized operations of g.atomicstatus to synchronized operations.

Updates #53821

Change-Id: If249d62420ea09fbec39b570942f96c63669c333
Reviewed-on: https://go-review.googlesource.com/c/go/+/425363
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit that referenced this issue Sep 5, 2022
@cuiweixie @mvdan
runtime: convert local var stop,ready at TestDebugCallUnsafePoint to …
357b922 
…atomic type

For #53821

Change-Id: Id972d4ccadc72de69dea46f8be146c9843d1d095
Reviewed-on: https://go-review.googlesource.com/c/go/+/427135
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: xie cui <523516579@qq.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Sep 5, 2022
@cuiweixie @mvdan
runtime: convert local var ready at TestMemmoveAtomicity to atomic type
7d83d32 
For #53821

Change-Id: I2487b8d18a4cd3fc6e64fbbb531419812bfe0f08
Reviewed-on: https://go-review.googlesource.com/c/go/+/427136
Run-TryBot: xie cui <523516579@qq.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit that referenced this issue Sep 5, 2022
@cuiweixie @mvdan
runtime: convert local var cunlock at doTestParallelReaders to atomic…
02700e5 
… type

For #53821

Change-Id: I17440ea30827976a8d3755851a2496f26aea13b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/427137
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: xie cui <523516579@qq.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit that referenced this issue Sep 5, 2022
@cuiweixie @mvdan
runtime: convert local var started,progress at TestStackGrowth to ato…
4ad55cd 
…mic type

For #53821

Change-Id: I9c777ff642ea4b70073335279551cea6a2394569
Reviewed-on: https://go-review.googlesource.com/c/go/+/427138
Run-TryBot: xie cui <523516579@qq.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit that referenced this issue Sep 5, 2022
@cuiweixie @mvdan
runtime: convert local var stop at TestAfterStress to atomic type
af7f417 
For #53821

Change-Id: I7e86dac34691f7752f68879ff379061f3435cd45
Reviewed-on: https://go-review.googlesource.com/c/go/+/427139
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: xie cui <523516579@qq.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@cuishuang cuishuang mentioned this issue Sep 6, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/428477 mentions this issue: crypto/rand: convert r.used to atomic type

@cuishuang cuishuang mentioned this issue Sep 6, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/428576 mentions this issue: sumdb: convert variable to atomic type

@cuishuang cuishuang mentioned this issue Sep 7, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/428919 mentions this issue: crypto/tls: convert Conn.activeCall to atomic type

@cuishuang cuishuang mentioned this issue Sep 7, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/428920 mentions this issue: compile/internal: convert Named.state_ to atomic type

gopherbot pushed a commit that referenced this issue Sep 7, 2022
@cuishuang @gopherbot
internal: convert calls to atomic type
cdc6aeb 
For #53821

Change-Id: Iee8ccea714726bbb6a4b384887bb107c29b823a9
GitHub-Last-Rev: 119aad3
GitHub-Pull-Request: #54862
Reviewed-on: https://go-review.googlesource.com/c/go/+/428335
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Keith Randall <khr@google.com>
gopherbot pushed a commit that referenced this issue Sep 9, 2022
@cuishuang @neild
net/http: convert allowQuerySemicolonsInUse to atomic type
b244b89 
For #53821

Change-Id: I135783bd5472011f6a74d2f5ee34ce96ff49ad2b
GitHub-Last-Rev: 4da2d67
GitHub-Pull-Request: #54863
Reviewed-on: https://go-review.googlesource.com/c/go/+/428336
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@prattmic
Copy link
Member Author

I did some initial benchmarking with x/bench/sweet on linux-arm64 to look for potential effects of these changes.

As of 0e16d67 (Aug 29), I see (vs 1.19):

name                                old time/op            new time/op            delta
BiogoIgor                                      11.7s ± 1%             11.2s ± 1%  -4.30%  (p=0.000 n=10+9)
BiogoKrishna                                   16.1s ± 0%             16.0s ± 0%  -0.41%  (p=0.000 n=10+10)
BleveIndexBatch100                             5.21s ± 3%             5.05s ± 1%  -2.98%  (p=0.000 n=10+10)
BleveQuery                                     2.45s ± 0%             2.46s ± 1%  +0.35%  (p=0.035 n=9+10)
FoglemanPathTraceRenderGopherIter1             18.5s ± 3%             18.4s ± 2%    ~     (p=0.796 n=10+10)
GopherLuaKNucleotide                           27.5s ± 1%             28.4s ± 1%  +3.13%  (p=0.000 n=10+10)
MarkdownRenderXHTML                            232ms ± 1%             236ms ± 1%  +1.43%  (p=0.000 n=9+10)
[Geo mean]                                     6.01s                  5.98s       -0.49%

name                                old average-RSS-bytes  new average-RSS-bytes  delta
BiogoIgor                                     67.5MB ± 5%            65.4MB ± 5%    ~     (p=0.105 n=10+10)
BiogoKrishna                                  4.19GB ± 0%            4.12GB ± 0%  -1.58%  (p=0.000 n=8+8)
BleveIndexBatch100                             202MB ± 2%             200MB ± 3%    ~     (p=0.165 n=10+10)
BleveQuery                                     535MB ± 1%             534MB ± 0%    ~     (p=0.905 n=10+9)
FoglemanPathTraceRenderGopherIter1             138MB ± 1%             137MB ± 2%  -1.09%  (p=0.043 n=10+10)
GopherLuaKNucleotide                          39.3MB ± 5%            38.7MB ± 3%    ~     (p=0.182 n=10+9)
MarkdownRenderXHTML                           22.2MB ± 7%            21.6MB ± 5%    ~     (p=0.133 n=10+9)
[Geo mean]                                     167MB                  165MB       -1.60%

name                                old peak-RSS-bytes     new peak-RSS-bytes     delta
BiogoIgor                                     92.3MB ± 4%            89.7MB ± 3%  -2.86%  (p=0.007 n=10+9)
BiogoKrishna                                  4.56GB ± 0%            4.49GB ± 0%  -1.50%  (p=0.000 n=10+10)
BleveIndexBatch100                             287MB ± 3%             285MB ± 4%    ~     (p=0.497 n=9+10)
BleveQuery                                     536MB ± 1%             536MB ± 0%    ~     (p=0.436 n=10+10)
FoglemanPathTraceRenderGopherIter1             185MB ± 1%             183MB ± 1%  -1.04%  (p=0.023 n=10+10)
GopherLuaKNucleotide                          44.2MB ± 7%            45.4MB ± 4%    ~     (p=0.356 n=9+10)
MarkdownRenderXHTML                           23.0MB ± 6%            22.3MB ± 9%    ~     (p=0.143 n=10+10)
[Geo mean]                                     198MB                  197MB       -0.88%

name                                old peak-VM-bytes      new peak-VM-bytes      delta
BiogoIgor                                      807MB ± 0%             804MB ± 0%  -0.27%  (p=0.000 n=10+10)
BiogoKrishna                                  5.31GB ± 0%            5.24GB ± 0%  -1.30%  (p=0.000 n=10+9)
BleveIndexBatch100                            2.39GB ± 0%            2.39GB ± 0%  -0.16%  (p=0.000 n=9+10)
BleveQuery                                    3.71GB ± 0%            3.66GB ± 3%  -1.40%  (p=0.003 n=8+10)
FoglemanPathTraceRenderGopherIter1             876MB ± 0%             873MB ± 0%  -0.33%  (p=0.000 n=10+10)
GopherLuaKNucleotide                           735MB ± 0%             734MB ± 0%  -0.13%  (p=0.000 n=10+10)
MarkdownRenderXHTML                            734MB ± 0%             733MB ± 0%  -0.15%  (p=0.000 n=10+10)
[Geo mean]                                    1.51GB                 1.50GB       -0.54%

Most of this improvement is likely from https://go.dev/cl/422634.

I intend to further investigate GopherLuaKNucleotide and MarkdownRenderXHTML to determine the source of regression.

gopherbot pushed a commit that referenced this issue Sep 13, 2022
@cuishuang @gopherbot
runtime: convert timer.status to atomic type
f78efc0 
For #53821

Change-Id: I7cb6a16626964d5023b96609d9921bfe4a679d8f
GitHub-Last-Rev: ddfce12
GitHub-Pull-Request: #54865
Reviewed-on: https://go-review.googlesource.com/c/go/+/428196
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Jenny Rakoczy <jenny@golang.org>
Auto-Submit: Jenny Rakoczy <jenny@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Oct 1, 2022
@cuishuang @mvdan
crypto/rand: convert r.used to atomic type
5f566d3 
For #53821

Change-Id: I1b5c62288eca20ff50f6d8d979cf82df24d4545b
GitHub-Last-Rev: 2661485
GitHub-Pull-Request: #54884
Reviewed-on: https://go-review.googlesource.com/c/go/+/428477
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Nov 10, 2022
@cuishuang @gopherbot
expvar: convert f to atomic type
d2aa787 
For #53821

Change-Id: I2e7c5376e6ca3e3dbb2f92ad771aed62fca8b793
GitHub-Last-Rev: b67ddf8
GitHub-Pull-Request: #54864
Reviewed-on: https://go-review.googlesource.com/c/go/+/428195
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@prattmic prattmic

Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests
2 participants
@prattmic @gopherbot