Skip to content

features/pool: clear repeated bytes/string field in ResetVT #152

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

Merged
merged 4 commits into from
Mar 13, 2025

Conversation

dethi
Copy link
Contributor

@dethi dethi commented Mar 11, 2025

Clearing repeated bytes/string field ensure that the memory can be
released by the garbage collector. Previously, the slice was only
resized to 0, but the underlying []byte/string were still referenced
by the slice.

String are immutable in Go and aren't supposed to be modified after they
are created, so reusing the allocated memory is not possible. The memory
allocated for []byte could be reused, but this requires some additional
modification to the generated Unmarshal code and would be incompatible
with the use of UnmarshalVTUnsafe. This could be considered in a
follow-up PR.

When repeating other types, we don't need to clear the slices:

  • scalar: the slice itself hold the memory
  • embedded messages: .Reset or .ResetVT is being called instead to
    clear the message.

clear() was introduced in Go 1.21, so the version has to be bumped.

dethi added 3 commits March 11, 2025 11:13
In newer version of Go, the SVC build information are always added by
default. Thus causing `make genall` to generate test files with a
different "protoc-gen-go-vtproto version" line.

Setting `-buildvcs=false` explicitely ensure that we keep using
`(devel)` for the version info.
Clearing repeated bytes/string field ensure that the memory can be
released by the garbage collector. Previously, the slice was only
resized to 0, but the underlying []byte/string were still referenced
by the slice.

String are immutable in Go and aren't supposed to be modified after they
are created, so reusing the allocated memory is not possible. The memory
allocated for []byte could be reused, but this requires some additional
modification to the generated Unmarshal code and would be incompatible
with the use of UnmarshalVTUnsafe. This could be considered in a
follow-up PR.

When repeating other types, we don't need to clear the slices:
- scalar: the slice itself hold the memory
- embedded messages: `.Reset` or `.ResetVT` is being called instead to
  clear the message.

`clear()` was introduced in Go 1.21, so the version has to be bumped.
Copy link
Contributor

@vmg vmg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this was indeed a memory leak. Very good catch!

Signed-off-by: Vicent Marti <[email protected]>
@vmg vmg merged commit ba97887 into planetscale:main Mar 13, 2025
@sbapu-arista
Copy link

We haven't had a release since Jan 2024. Can we do a new release please?

@tristan-instacart
Copy link

+1 to a release.

I was just doing prep to roll this out and would have hit this leak. Thank you for fixing @dethi and @vmg !

@dethi dethi deleted the pool-clear-slices branch April 24, 2025 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants