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

Use strings.Builder for v.String() #8

Closed
wants to merge 2 commits into from
Closed

Use strings.Builder for v.String() #8

wants to merge 2 commits into from

Conversation

kke
Copy link
Collaborator

@kke kke commented Sep 11, 2023

As mentioned in a go-version issue upstream, calling v.String() leaks memory.

This is an attempt to fix that for k0sproject/version.

@kke kke added bug Something isn't working enhancement New feature or request labels Sep 11, 2023
@kke kke force-pushed the string-memleak branch 2 times, most recently from 84e1f87 to 9e7f98e Compare September 11, 2023 11:24
Signed-off-by: Kimmo Lehto <[email protected]>
@kke kke changed the title Fix v.String() memory leak Use strings.Builder for v.String() Jan 12, 2024
@kke kke marked this pull request as draft January 12, 2024 07:36
@kke
Copy link
Collaborator Author

kke commented Jan 12, 2024

After looking at it again, I don't think there's any "leak" upstream, it just allocates a string that needs to be garbage collected. This fork makes the problem way worse by doing a LOT of String()==String().

Sorting a long list will allocate a gazillion strings.

@kke
Copy link
Collaborator Author

kke commented Jan 12, 2024

Doesn't make it better.

@kke kke closed this Jan 12, 2024
@kke kke deleted the string-memleak branch January 12, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant