You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The memory profile is written after a GC should have freed up the zip.Reader. So it's surprising to see the memory allocated by os.ReadFile is still there.
CreateRaw stores the FileHeader pointer in the Writer w. The code that populates the FileHeader takes care to ensure that it shares no memory with the zip.Reader from which it came. Unfortunately, FileHeader is embedded in File, which does refer to the underlying reader:
type File struct {
FileHeader
...
zipr io.ReaderAt // the argument to NewReader
...
}
I believe the pointer to the embedded FileHeader causes the entire File struct to be retained, including the bytes used for the reader.
The problem can be worked around by copying the embedded FileHeader. Luckily, Writer.Copy is a convenience method whose functionality can be written outside the standard library:
I believe Writer.Copy should be fixed in this way. I don't think the fix should be in Writer.CreateRaw because users may depend on the identity of its argument. But its documentation should be updated to alert users to the problem.
The text was updated successfully, but these errors were encountered:
Make a copy of the argument File's FileHeader, and pass a pointer
to the copy to CreateRaw.
Passing the pointer directly causes the entire `File` to be referenced
by the receiver. The `File` includes a reference to the `ReaderAt`
underlying the `Reader`, so all its memory, which may be the entire
contents of the archive, is prevented from being garbage-collected.
Also, explain the issue in the doc comment for CreateRaw. We
cannot change its behavior because someone may depend on the
preserving the identity of its argument pointer.
For #65499.
Change-Id: Ieb4963a0ea30539d597547d3511accbd8c6b5c5a
Reviewed-on: https://go-review.googlesource.com/c/go/+/560238
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
ezz-no
pushed a commit
to ezz-no/go-ezzno
that referenced
this issue
Feb 18, 2024
Make a copy of the argument File's FileHeader, and pass a pointer
to the copy to CreateRaw.
Passing the pointer directly causes the entire `File` to be referenced
by the receiver. The `File` includes a reference to the `ReaderAt`
underlying the `Reader`, so all its memory, which may be the entire
contents of the archive, is prevented from being garbage-collected.
Also, explain the issue in the doc comment for CreateRaw. We
cannot change its behavior because someone may depend on the
preserving the identity of its argument pointer.
For golang#65499.
Change-Id: Ieb4963a0ea30539d597547d3511accbd8c6b5c5a
Reviewed-on: https://go-review.googlesource.com/c/go/+/560238
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Go version
go1.22-20240109-RC01 linux/amd64
Output of
go env
in your module/workspace:What did you do?
The following program is
zipmem.go
:Shell command:
Inside
pprof
:What did you see happen?
What did you expect to see?
The memory profile is written after a GC should have freed up the
zip.Reader
. So it's surprising to see the memory allocated byos.ReadFile
is still there.I believe the reason is this line in
Writer.Copy
:CreateRaw
stores the FileHeader pointer in the Writerw
. The code that populates the FileHeader takes care to ensure that it shares no memory with thezip.Reader
from which it came. Unfortunately,FileHeader
is embedded inFile
, which does refer to the underlying reader:I believe the pointer to the embedded
FileHeader
causes the entireFile
struct to be retained, including the bytes used for the reader.The problem can be worked around by copying the embedded
FileHeader
. Luckily,Writer.Copy
is a convenience method whose functionality can be written outside the standard library:I believe
Writer.Copy
should be fixed in this way. I don't think the fix should be inWriter.CreateRaw
because users may depend on the identity of its argument. But its documentation should be updated to alert users to the problem.The text was updated successfully, but these errors were encountered: