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

Unexpected memory usage #851

Closed
wyxloading opened this issue Jul 27, 2020 · 3 comments
Closed

Unexpected memory usage #851

wyxloading opened this issue Jul 27, 2020 · 3 comments

Comments

@wyxloading
Copy link
Contributor

Here cloned a jsonEncoder (which actually get from _jsonPool), but never put back to _jsonPool after use.

func (c consoleEncoder) writeContext(line *buffer.Buffer, extra []Field) {
context := c.jsonEncoder.Clone().(*jsonEncoder)
defer context.buf.Free()
addFields(context, extra)
context.closeOpenNamespaces()
if context.buf.Len() == 0 {
return
}
c.addSeparatorIfNecessary(line)
line.AppendByte('{')
line.Write(context.buf.Bytes())
line.AppendByte('}')
}

Perhaps we need something like below to decrease memory allocations.

context := c.jsonEncoder.Clone().(*jsonEncoder) 
defer func() {
    context.buf.Free()
    putJSONEncoder(context)
}
@prashantv
Copy link
Collaborator

From a quick glance, this seems reasonable. Can you send a PR, and ensure the race detector doesn't find any issues once the above change is made. Thanks!

wyxloading pushed a commit to wyxloading/zap that referenced this issue Jul 28, 2020
consoleEncoder clone a jsonEncoder in `writeContext`, but never put back to pool after use.
This make zap do more memory allocations, and may increase gc time.
wyxloading pushed a commit to wyxloading/zap that referenced this issue Jul 28, 2020
consoleEncoder clone a jsonEncoder in `writeContext`, but never put back to pool after use.
This make zap do more memory allocations, and may increase gc time.
wyxloading added a commit to wyxloading/zap that referenced this issue Jul 28, 2020
consoleEncoder clone a jsonEncoder in `writeContext`, but never put back to pool after use.
This make zap do more memory allocations, and may increase gc time.
@wyxloading
Copy link
Contributor Author

I would like to send PR for this (FYI, PR link #852)

@abhinav
Copy link
Collaborator

abhinav commented Dec 3, 2020

#852 was merged so we can resolve this.

@abhinav abhinav closed this as completed Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants