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

container/heap: confusing claim of memory leak when shrinking slices #65403

Closed
codesoap opened this issue Jan 31, 2024 · 4 comments
Closed

container/heap: confusing claim of memory leak when shrinking slices #65403

codesoap opened this issue Jan 31, 2024 · 4 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@codesoap
Copy link
Contributor

Go version

go version go1.21.1 openbsd/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/richard/.cache/go-build'
GOENV='/home/richard/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='openbsd'
GOINSECURE=''
GOMODCACHE='/home/richard/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='openbsd'
GOPATH='/home/richard/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/openbsd_amd64'
GOVCS=''
GOVERSION='go1.21.1'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build50129197=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I read the "PriorityQueue" example of the container/heap documentation.

What did you see happen?

I came across this line, claiming to prevent a memory leak. It confused, me since I've never heard of a memory leak caused by Go slices.

Upon some pondering, I figured that the author of this line is talking about the memory, that is retained when shrinking a slice. However, I think the term "memory leak" is not right for this, since the memory will be freed once the slice becomes unused or it will be reused when the slice grows again. At most, I would call this a "space leak".

What did you expect to see?

I would prefer this line to be absent. It seems like a premature optimization to me and doesn't help explaining the container/heap package.
@codesoap codesoap mentioned this issue Jan 31, 2024
Closed
@gopherbot
Copy link

Change https://go.dev/cl/559775 mentions this issue: container/heap: remove confusing claim of memory leak

@mknyszek mknyszek changed the title container/heap: Confusing claim of memory leak when shrinking slices container/heap: confusing claim of memory leak when shrinking slices Jan 31, 2024
@mknyszek
Copy link
Contributor

This is about a comment internal to the standard library, not any actual user-facing documentation.

You do not need to file an issue for this sort of thing in the future. You can just send a change and it will get reviewed. If you're unsure whether it's worth the effort, start a thread on https://groups.google.com/g/golang-dev first.

Thanks. Closing this issue.

@mknyszek mknyszek closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2024
@codesoap
Copy link
Contributor Author

Alright thanks, I'll remember it. However, this is indeed user-facing documentation. It is part of an example, that is visible here: https://pkg.go.dev/container/heap@go1.21.6#example-package-PriorityQueue

@mknyszek
Copy link
Contributor

Oh! Thank you. My apologies for not looking closely enough.

@mknyszek mknyszek reopened this Jan 31, 2024
@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 31, 2024
@mknyszek mknyszek added this to the Backlog milestone Jan 31, 2024
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@codesoap @ezz-no
container/heap: remove confusing claim of memory leak
32d17b7 
The term "memory leak" was misused here, as the memory is still referenced
by the slice.

Fixes golang#65403

Change-Id: Id102419d4c798fb2a4ec8be86be9ec9b5cdd98e6
GitHub-Last-Rev: 3febcd0
GitHub-Pull-Request: golang#65404
Reviewed-on: https://go-review.googlesource.com/c/go/+/559775
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Keith Randall <khr@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

container/heap: remove confusing claim of memory leak codesoap/go
3 participants
@mknyszek @gopherbot @codesoap