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

Add Buffer.WriteByte and WriteString methods #691

Merged
merged 2 commits into from Jun 24, 2021

Conversation

smira
Copy link
Contributor

@smira smira commented Mar 25, 2019

These methods are equivalent to AppendByte and AppendString, but
their signatures are compatible with bytes.Buffer and bufio.Writer.

This allows to use Buffer where bytes.Buffer was expected without
extra cost of wrapping. One example is msgpack library which
expects Writer to implement WriteByte and WriteString, otherwise
it is wrapped which incurs extra allocation:
https://github.com/vmihailenco/msgpack/blob/master/encode.go#L63-L67

These methods are equivalent to `AppendByte` and `AppendString`, but
their signatures are compatible with
[bytes.Buffer](https://godoc.org/bytes#Buffer) and
[bufio.Writer](https://godoc.org/bufio#Writer).

This allows to use `Buffer` where `bytes.Buffer` was expected without
extra cost of wrapping. One example is msgpack library which
expects `Writer` to implement `WriteByte` and `WriteString`, otherwise
it is wrapped which incurs extra allocation:
https://github.com/vmihailenco/msgpack/blob/master/encode.go#L63-L67
@CLAassistant
Copy link

CLAassistant commented Mar 25, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #691 (3c8c05c) into master (fb71758) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #691   +/-   ##
=======================================
  Coverage   98.07%   98.07%           
=======================================
  Files          45       45           
  Lines        2027     2031    +4     
=======================================
+ Hits         1988     1992    +4     
  Misses         30       30           
  Partials        9        9           
Impacted Files Coverage Δ
buffer/buffer.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 fb71758...3c8c05c. Read the comment docs.

smira added a commit to smira/zap-msgpack-encoder that referenced this pull request Mar 26, 2019
We anyway need to cache in sync.Pool instances of msgpack Encoder
object, and this object is bound to Writer instance (Buffer in our
case). There's no way to reset encoder to use different buffer, and
there's no way to avoid buffer from being recycled if returned, so it's
easier to use bytes.Buffer which has necessary APIs to avoid extra
allocations (see uber-go/zap#691).

The only allocation left in the benchmark is from msgpack's EncodeTime
function, PR is submitted for it: vmihailenco/msgpack#224
@prashantv prashantv merged commit 5afb307 into uber-go:master Jun 24, 2021
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants