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

strings: strings.Builder case memory leak #34463

Closed
szyhf opened this issue Sep 23, 2019 · 9 comments
Closed

strings: strings.Builder case memory leak #34463

szyhf opened this issue Sep 23, 2019 · 9 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@szyhf
Copy link

szyhf commented Sep 23, 2019

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

Tried both 1.13.0 and 1.12.9.

$ go version

Does this issue reproduce with the latest release?

What operating system and processor architecture are you using (go env)?

In docker image alpine:3.8, build cmd is GOOS=linux GOARCH=amd64 go build on macOS 10.14.6

go env Output
$ go env

What did you do?

Using a sync.Pool to reuse strings.Builder.

https://play.golang.org/p/TkWbxGu4PRJ

I use strings.Builder to build sql, and exec the sql-str every times, but I found that the memory will increase slowly, then I called pprof/heap and found that the strings.(*Builder).WriteString inuse a lot of memory and looks never released.

QQ20190923-085002

What did you expect to see?

The memory not increase unexpectedly

What did you see instead?

The memory not increase unexpectedly

Really thanks for help!

@szyhf szyhf changed the title strings: strings.Builder case memory leak strings: strings.Builder with sync.Pool case memory leak Sep 23, 2019
@ianlancetaylor
Copy link
Contributor

Please show us more of the program. The memory profile you show is exactly what I would expect if your program never releases the strings built with strings.Builder.

That said, while there may be a bug here somewhere, it does not make sense to put strings.Builder values into a sync.Pool. Allocating a new strings.Builder costs nothing. This is not a case where you should be using a sync.Pool.

@toothrot toothrot added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 23, 2019
@toothrot toothrot added this to the Go1.14 milestone Sep 23, 2019
@szyhf
Copy link
Author

szyhf commented Sep 23, 2019

Please show us more of the program. The memory profile you show is exactly what I would expect if your program never releases the strings built with strings.Builder.

That said, while there may be a bug here somewhere, it does not make sense to put strings.Builder values into a sync.Pool. Allocating a new strings.Builder costs nothing. This is not a case where you should be using a sync.Pool.


Thanks for help.

Yes, you are right, when I read the detail code of strings.Builder I recognized that it's different from bytes.Buffer, so I remove the sync.Pool of strings.Builder hours ago and upgrade our prod application and hope if it help.


We are using https://github.com/astaxie/beego/tree/develop/orm to exec sql, the var ormer in demo is orm.Ormer config as mysql, and I skip the init of it.

The main target of the function is to build a sql like insert into test_tbl (id,dr) values (1,'2'),(2,'3'),...(x,'y') on duplicate key update dr=values(dr);

The demo of our function is here: https://play.golang.org/p/o0cEeZ7o7xl

The function is called every 3min by a time.Ticker.

The memory increase really slow, the memory monitor of previous 7 days is blow:

QQ20190923-112925

This case me to read heap to looking for what is leaking.

Thanks again~

@martisch
Copy link
Contributor

Closing as this looks to be resolved and working as intended.

@szyhf
Copy link
Author

szyhf commented Sep 27, 2019

Hi, @martisch @ianlancetaylor

I've tried some solution and I think I found the leaking code, but I don't know why(I think it's a bug).

First solution, I remove the usage of sync.Pool, I create a new strings.Builder every time, and the leaking is still slow but continues.

// Key code of first solution
sb := new(strings.Builder)

// full code writing ref: https://play.golang.org/p/o0cEeZ7o7xl
// sb.WriteString(...)
// sb.WriteRune(...)

_, err := ormer.Raw(strings.TrimSuffix(sb.String(), ","), sqlParamList...).Exec()

The second solution worked and the memory is not leaking.

// Key code of second solution

sb := new(strings.Builder)

// full code writing ref: https://play.golang.org/p/o0cEeZ7o7xl
// sb.WriteString(...)
// sb.WriteRune(...)

sql := sb.String()
sql = strings.TrimSuffix(sql, ",")
sb.Reset()
sb = nil
_, err := ormer.Raw(sql, sqlParamList...).Exec()

Here is the memory usage curve:

QQ20190927-093834

I don't really understand why the first solution will case the strings.Builder not release the memory ?

Here is the heap of first solution:

QQ20190924-175638

Here is the heap of second solution:

QQ20190927-095021

@ianlancetaylor
Copy link
Contributor

How are you measuring memory use?

@szyhf
Copy link
Author

szyhf commented Sep 27, 2019

Hi @ianlancetaylor

The curve is provide by monitor-agent of Aliyun ECS(Elastic Compute Service), is the memory usage of the ECS.

The ECS only run the app-server on docker , and I've use htop via ssh to check the memory of the process, the memory is really high-load on the app-server.


About our usage:

We are running a same app-server for different customers on different ECS, some has a lot users and some are not.

Firstly, I found the memory keep high-load on a app-server with lots users, and I found the memory usage going up unusual by curve, than I tried pprof for heap and found the strings.Builder.WriteString() keeps a lot of memory.

Cause of our business needs, I can't test the solution on that server, I use a much smaller server with less users to test the first solution and second solution.

The smaller server has the some leaking too, but less users makes it much more slowly, the heap shows that the leaking is on strings.Builder too.


Thanks for help, let me know If any other information I can provide,.

@szyhf
Copy link
Author

szyhf commented Sep 28, 2019

Hi, @martisch @ianlancetaylor, here is the additional information:

I tried another code in another server:

sql := sb.String()
sql = strings.TrimSuffix(sql, ",")
_, err := ormer.Raw(sql, sqlParamList...).Exec()

Just a little different of the second solution, and the leak is shown again.

Here is the heap:

QQ20190929-071127

And here is the heap for second solution running till now ( no leaking and stable):

QQ20190929-071422

@szyhf szyhf changed the title strings: strings.Builder with sync.Pool case memory leak strings: strings.Builder case memory leak Sep 29, 2019
@szyhf
Copy link
Author

szyhf commented Sep 29, 2019

Really sorry that I made a mistake that set sb = nil of the second solution case the server panic and auto-restart every hour, that case the memory stable and not sure to solve problem, I'm running the fix server and waiting for new heap ……

@szyhf
Copy link
Author

szyhf commented Oct 8, 2019

Just reset strings.Builder can not solve it.

@golang golang locked and limited conversation to collaborators Oct 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

5 participants