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: total allocation stats are managed in a uintptr which can quickly wrap around on 32-bit architectures #52680

Closed
mknyszek opened this issue May 3, 2022 · 7 comments
Closed

runtime: total allocation stats are managed in a uintptr which can quickly wrap around on 32-bit architectures #52680

mknyszek opened this issue May 3, 2022 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages)
Milestone
Go1.19

Comments

@mknyszek
Copy link
Contributor

mknyszek commented May 3, 2022

A runtime failure occurs on an assertion in ReadMemStats when running the internal/intern tests. I can reproduce it locally via:

GOARCH=386 go test internal/intern

On the 386 longtest builder: https://build.golang.org/log/38f434ccb5ed0db84e99122ef69eb1f3894d7d33

Curiously, it doesn't reproduce on amd64, AFAICT.
@mknyszek mknyszek added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages) labels May 3, 2022
@mknyszek mknyszek added this to the Go1.19 milestone May 3, 2022
@mknyszek mknyszek self-assigned this May 3, 2022
@mknyszek
Copy link
Contributor Author

mknyszek commented May 3, 2022

I understand the problem. This test allocates a whole lot of memory in a loop, > 4 GiB, and memstats.heapStats.largeAlloc is unfortunately a uintptr. It really should be a uint64. It's not the only one with this problem.

It's an easy fix and we should probably backport it.

@mknyszek mknyszek changed the title runtime: totalAlloc and consistent stats are not equal runtime: total allocation stats are managed in a uintptr which can quickly wrap around on 32-bit architectures May 3, 2022
@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label May 3, 2022
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 3, 2022
@gopherbot
Copy link

Change https://go.dev/cl/403758 mentions this issue: runtime: store consistent total allocation stats as uint64

@mknyszek
Copy link
Contributor Author

mknyszek commented May 3, 2022

@gopherbot Please open backport issues for Go 1.17 and Go 1.18.

@gopherbot gopherbot mentioned this issue May 3, 2022
Closed
@gopherbot
Copy link

Backport issue(s) opened: #52688 (for 1.17), #52689 (for 1.18).

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 May 3, 2022
Closed
@bcmills bcmills mentioned this issue May 4, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/411495 mentions this issue: [release-branch.go1.18] runtime: store consistent total allocation stats as uint64

@gopherbot
Copy link

Change https://go.dev/cl/411494 mentions this issue: [release-branch.go1.18] runtime: store consistent total allocation stats as uint64

@gopherbot
Copy link

Change https://go.dev/cl/411496 mentions this issue: [release-branch.go1.17] runtime: store consistent total allocation stats as uint64

gopherbot pushed a commit that referenced this issue Jul 6, 2022
@mknyszek @heschi
[release-branch.go1.17] runtime: store consistent total allocation st…
b1be664 
…ats as uint64

Currently the consistent total allocation stats are managed as uintptrs,
which means they can easily overflow on 32-bit systems. Fix this by
storing these stats as uint64s. This will cause some minor performance
degradation on 32-bit systems, but there really isn't a way around this,
and it affects the correctness of the metrics we export.

For #52680.
Fixes #52688.

Change-Id: I8b1926116e899ae9f03d58e0320bcb9264945b3e
Reviewed-on: https://go-review.googlesource.com/c/go/+/411496
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
gopherbot pushed a commit that referenced this issue Jul 6, 2022
@mknyszek @heschi
[release-branch.go1.18] runtime: store consistent total allocation st…
f0eca66 
…ats as uint64

Currently the consistent total allocation stats are managed as uintptrs,
which means they can easily overflow on 32-bit systems. Fix this by
storing these stats as uint64s. This will cause some minor performance
degradation on 32-bit systems, but there really isn't a way around this,
and it affects the correctness of the metrics we export.

For #52680.
Fixes #52689.

Change-Id: I8b1926116e899ae9f03d58e0320bcb9264945b3e
Reviewed-on: https://go-review.googlesource.com/c/go/+/411495
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 14, 2022
@mknyszek @bradfitz
[release-branch.go1.18] runtime: store consistent total allocation st…
ee62c13 
…ats as uint64

Currently the consistent total allocation stats are managed as uintptrs,
which means they can easily overflow on 32-bit systems. Fix this by
storing these stats as uint64s. This will cause some minor performance
degradation on 32-bit systems, but there really isn't a way around this,
and it affects the correctness of the metrics we export.

For golang#52680.
Fixes golang#52689.

Change-Id: I8b1926116e899ae9f03d58e0320bcb9264945b3e
Reviewed-on: https://go-review.googlesource.com/c/go/+/411495
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
@golang golang locked and limited conversation to collaborators Jun 22, 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. release-blocker Soon This needs to be done soon. (regressions, serious bugs, outages)
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
2 participants
@mknyszek @gopherbot