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: performance regression due to bad instruction used in morestack_noctxt for ppc64 in CL 425396 #57741

Closed
laboger opened this issue Jan 11, 2023 · 7 comments
Closed

runtime: performance regression due to bad instruction used in morestack_noctxt for ppc64 in CL 425396 #57741

laboger opened this issue Jan 11, 2023 · 7 comments
Assignees
@archanaravindar
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.20

Comments

@laboger
Copy link
Contributor

laboger commented Jan 11, 2023

What version of Go are you using (go version)?

$ go version
latest

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
ppc64le power10

What did you do?

What did you expect to see?

Consistent performance of runtime benchmarks.

What did you see instead?

Some benchmarks in the runtime package show a 3X or more degradation on power10 after CL 425396 was merged. Removing this line in the latest also removes the degradation. For example, Hash65536 is one example.

name          old time/op    new time/op   delta
Hash65536-64    4.08µs ± 2%  18.89µs ±25%  +363.16%  (p=0.029 n=4+4)

name          old speed      new speed     delta
Hash65536-64  16.1GB/s ± 2%  3.5GB/s ±22%   -77.96%  (p=0.029 n=4+4)

The MOVD R1, R1 results in
or r1,r1,r1
which is a special instruction on ppc64 that changes thread priority. This is described in section 3.2 of ISA 3.1.

Changing it to OR $0, R1 has the same effect wrt SPWRITE but does not affect thread priority.

@cherrymui @pmur @archanaravindar
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 11, 2023
@cherrymui cherrymui added this to the Go1.20 milestone Jan 11, 2023
@cherrymui
Copy link
Member

Interesting. Good finding!
Mind sending a CL? Or I could send one, but I probably won't be able to validate the performance impact myself. Thanks.

@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 11, 2023
@laboger
Copy link
Contributor Author

laboger commented Jan 11, 2023

We can do it. We have been testing it out.

@gopherbot
Copy link

Change https://go.dev/cl/461597 mentions this issue: runtime: fix performance regression in morestack_noctxt on ppc64

@archanaravindar archanaravindar self-assigned this Jan 11, 2023
@laboger laboger removed their assignment Jan 11, 2023
@laboger
Copy link
Contributor Author

laboger commented Jan 13, 2023

@cherrymui we want this in Go 1.20, so is there anything special needed on the CL now that the tree is open for 1.21?

@cherrymui
Copy link
Member

The tree hasn't opened yet. So the CL can be submitted as usual.

@archanaravindar
Copy link
Contributor

archanaravindar commented Jan 16, 2023 •
edited

@gopherbot please backport this to previous releases Go1.19 and Go1.18
Since the fix 54332 that causes this regression on ppc64 has been likewise back ported to these two releases
@laboger @cherrymui @pmur

@gopherbot gopherbot mentioned this issue Jan 16, 2023
Closed
@gopherbot
Copy link

Backport issue(s) opened: #57811 (for 1.18), #57812 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot gopherbot mentioned this issue Jan 16, 2023
Closed
@golang golang locked and limited conversation to collaborators Jan 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@archanaravindar archanaravindar

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
4 participants
@gopherbot @laboger @archanaravindar @cherrymui