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 ByteString API #366

Merged
merged 6 commits into from
Mar 14, 2017
Merged

Add ByteString API #366

merged 6 commits into from
Mar 14, 2017

Conversation

skipor
Copy link
Contributor

@skipor skipor commented Mar 12, 2017

This is a breaking change that adds bytestring APIs (to log UTF-8 encoded bytes) to ObjectEncoder and ArrayEncoder. To actually save allocations along this path, I also benchmarked adding a []byte to the field union; this reduces the allocation count for bytestrings, but increases the cost of adding fields by ~50%.

Fixes #324.

@akshayjshah
Copy link
Contributor

This is great - nicely done.

I'm going to push a small change (less use of named returns), and I'm going to benchmark whether it's worth adding a []byte to the Field union struct.

@akshayjshah akshayjshah force-pushed the bytestring branch 2 times, most recently from 45004b1 to c7ba680 Compare March 14, 2017 21:25
This is a breaking change that adds a `AddByteString` to `zapcore.ObjectEncoder`, `AppendByteString` to `zapcore.PrimitiveArrayEncoder`.
This API optimizes logging UTF-8 encoded []byte data.

Fixes uber-go#324.
skipor and others added 5 commits March 14, 2017 14:26
This is a breaking change that adds a `AddByteString` to `zapcore.ObjectEncoder`, `AppendByteString` to `zapcore.PrimitiveArrayEncoder`.
This API optimizes logging UTF-8 encoded []byte data.

Fixes uber-go#324.
To make logging bytestrings and binary more efficient, add a byte slice to the
Field union. This adds ~80ns to the `AddingFields` benchmarks, but saves an
allocation along a path that particularly performance-sensitive applications
will use.
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