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

Pool the buffer and encoder used for generic JSON reflection #602

Merged
merged 5 commits into from
Jun 27, 2018

Conversation

jcorbin
Copy link
Contributor

@jcorbin jcorbin commented Jun 18, 2018

No description provided.

@jcorbin jcorbin requested review from prashantv and akshayjshah June 18, 2018 19:58
@codecov
Copy link

codecov bot commented Jun 18, 2018

Codecov Report

Merging #602 into master will decrease coverage by 0.17%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #602      +/-   ##
==========================================
- Coverage   97.47%   97.29%   -0.18%     
==========================================
  Files          39       39              
  Lines        2017     2035      +18     
==========================================
+ Hits         1966     1980      +14     
- Misses         43       47       +4     
  Partials        8        8
Impacted Files Coverage Δ
buffer/buffer.go 87.09% <0%> (-12.91%) ⬇️
zapcore/json_encoder.go 100% <100%> (ø) ⬆️

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 88c71ae...93e77b2. Read the comment docs.

@jcorbin jcorbin force-pushed the pool_reflected_buf branch from ae8d2a1 to 930a571 Compare June 18, 2018 20:15
Copy link
Contributor

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

This is awesome - great catch, Josh. One question about adding more mostly-duplicate tests. (Once again, my grand plan to split the zap and zapcore packages turns out to have been a bad idea.)

buf.Free()
})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar tests already exist in the top-level zap package; would we be better off changing the Makefile to pass coverpkg=all instead of adding a new test suite?

Copy link
Contributor

Choose a reason for hiding this comment

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

For future reviewers: this turned into a yak shave, will fix it later.

@jcorbin jcorbin force-pushed the pool_reflected_buf branch from 13daf8a to 9affa89 Compare June 20, 2018 17:30
@jcorbin
Copy link
Contributor Author

jcorbin commented Jun 20, 2018

Hmm, that -coverpkg=all trick didn't get me a greenlight from the bot; should I revert back to the new test?

@jcorbin jcorbin force-pushed the pool_reflected_buf branch from 9affa89 to 63a5674 Compare June 26, 2018 19:52
@jcorbin jcorbin force-pushed the pool_reflected_buf branch from 63a5674 to 93e77b2 Compare June 26, 2018 19:59
@akshayjshah akshayjshah merged commit f4243df into master Jun 27, 2018
@akshayjshah akshayjshah deleted the pool_reflected_buf branch June 27, 2018 03:28
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
…#602)

Pool buffers and encoders to make JSON reflection less expensive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants