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

x/text/cases: Is there memory leak in icu.go? #25562

Closed
caoruidong opened this issue May 25, 2018 · 3 comments
Closed

x/text/cases: Is there memory leak in icu.go? #25562

caoruidong opened this issue May 25, 2018 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@caoruidong
Copy link

caoruidong commented May 25, 2018 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.10.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

We take a look at golang/text/cases/icu.go. Find that cgo is used in this file.

What did you expect to see?

C.CString freed.

What did you see instead?

loc and src are not freed.
https://github.com/golang/text/blob/5c1cf69b5978e5a34c5f9ba09a83e56acc4b7877/cases/icu.go#L29

loc := C.CString(tag)
cm := C.ucasemap_open(loc, C.uint32_t(0), &err)
buf := make([]byte, len(input)*4)
dst := (*C.char)(unsafe.Pointer(&buf[0]))
src := C.CString(input)
@gopherbot gopherbot added this to the Unreleased milestone May 25, 2018
@agnivade
Copy link
Contributor

/cc @mpvl

@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 27, 2018
@meirf
Copy link
Contributor

meirf commented Jul 20, 2018

@caoruidong, there does seem like a leak but the only way to know for sure is to profile it. I wasn't able to get the usecasemap* functions working but here are some tips if you are able to:

I do think you should run e.g. doICU("lower", "af", "a\u05d0a") in a loop and observe the memory usage of that process from the perspective of the OS. For example, when running just C.CString vs C.CString + C.free on my mac I was able to clearly see not freeing caused memory usage to balloon. (I happen to have used the built in osx Activity Monitor).

I do not think putting e.g. doICU("lower", "af", "a\u05d0a") in a Benchmark and then using -benchmem will show you memory usage for the same reason go isn't able to garbage collect those strings - these string allocations are not known to Go's memory manager.

@caoruidong
Copy link
Author

caoruidong commented Nov 8, 2018 •
edited

When using cgo, memory is managed manually. From code, I don't see a call to free. Anyway this only exists in test code, so close it.

@golang golang locked and limited conversation to collaborators Nov 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@agnivade @meirf @caoruidong @gopherbot