Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

consoleEncoder: put cloned jsonEncoder back to pool #852

Merged
merged 2 commits into from
Aug 5, 2020

Conversation

wyxloading
Copy link
Contributor

This PR put back the cloned jsonEncoder in consoleEncoder to _jsonPool as mentioned in #851

Tested and got no errors ( with race detector )

  • make test
  • go test -race -run="^$$" -bench="BenchmarkZapConsole" ./zapcore

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2020

CLA assistant check
All committers have signed the CLA.

@wyxloading wyxloading force-pushed the jsonencoder_in_pool branch from bb9aed3 to 6b618ff Compare July 28, 2020 01:36
@codecov
Copy link

codecov bot commented Jul 28, 2020

Codecov Report

Merging #852 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #852   +/-   ##
=======================================
  Coverage   98.35%   98.35%           
=======================================
  Files          43       43           
  Lines        2368     2371    +3     
=======================================
+ Hits         2329     2332    +3     
  Misses         32       32           
  Partials        7        7           
Impacted Files Coverage Δ
zapcore/console_encoder.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53a3870...48ec7aa. Read the comment docs.

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.
zapcore/console_encoder.go Show resolved Hide resolved
Co-authored-by: Prashant Varanasi <github@prashantv.com>
Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @wyxloading!

@prashantv prashantv merged commit 639461d into uber-go:master Aug 5, 2020
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
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.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants