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.

Sign up for GitHub

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

Jump to bottom

JSON: Fix complex64 encoding precision #1011

Merged
merged 4 commits into from
Sep 10, 2021
Merged

JSON: Fix complex64 encoding precision #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b69a5a
JSON encoder: Test several corner cases
abhinav Sep 10, 2021
af0cbd1
JSON encoder: Use correct precision for Complex64
abhinav Sep 10, 2021
7255fee
CHANGELOG: Add entry for this bugfix
abhinav Sep 10, 2021
e272362
benchmark cost of encoding float32, complex64
abhinav Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,15 @@ All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## Unreleased
* No changes yet.

Bugfixes:
* [#1011][]: JSON: Fix inaccurate precision when encoding float64.

[#1011]: https://github.com/uber-go/zap/pull/1011

## 1.19.1 (8 Sep 2021)

### Fixed
Bugfixes:
* [#1001][]: JSON: Fix complex number encoding with negative imaginary part. Thanks to @hemantjadon.
* [#1003][]: JSON: Fix inaccurate precision when encoding float32.

Expand Down
58 changes: 33 additions & 25 deletions zapcore/json_encoder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ func (enc *jsonEncoder) AddComplex128(key string, val complex128) {
enc.AppendComplex128(val)
}

func (enc *jsonEncoder) AddComplex64(key string, val complex64) {
enc.addKey(key)
enc.AppendComplex64(val)
}

func (enc *jsonEncoder) AddDuration(key string, val time.Duration) {
enc.addKey(key)
enc.AppendDuration(val)
Expand Down Expand Up @@ -225,20 +230,23 @@ func (enc *jsonEncoder) AppendByteString(val []byte) {
enc.buf.AppendByte('"')
}

func (enc *jsonEncoder) AppendComplex128(val complex128) {
// appendComplex appends the encoded form of the provided complex128 value.
// precision specifies the encoding precision for the real and imaginary
// components of the complex number.
func (enc *jsonEncoder) appendComplex(val complex128, precision int) {
enc.addElementSeparator()
// Cast to a platform-independent, fixed-size type.
r, i := float64(real(val)), float64(imag(val))
enc.buf.AppendByte('"')
// Because we're always in a quoted string, we can use strconv without
// special-casing NaN and +/-Inf.
enc.buf.AppendFloat(r, 64)
enc.buf.AppendFloat(r, precision)
// If imaginary part is less than 0, minus (-) sign is added by default
// by AppendFloat.
if i >= 0 {
enc.buf.AppendByte('+')
}
enc.buf.AppendFloat(i, 64)
enc.buf.AppendFloat(i, precision)
enc.buf.AppendByte('i')
enc.buf.AppendByte('"')
}
Expand Down Expand Up @@ -301,28 +309,28 @@ func (enc *jsonEncoder) AppendUint64(val uint64) {
enc.buf.AppendUint(val)
}

func (enc *jsonEncoder) AddComplex64(k string, v complex64) { enc.AddComplex128(k, complex128(v)) }
func (enc *jsonEncoder) AddInt(k string, v int) { enc.AddInt64(k, int64(v)) }
func (enc *jsonEncoder) AddInt32(k string, v int32) { enc.AddInt64(k, int64(v)) }
func (enc *jsonEncoder) AddInt16(k string, v int16) { enc.AddInt64(k, int64(v)) }
func (enc *jsonEncoder) AddInt8(k string, v int8) { enc.AddInt64(k, int64(v)) }
func (enc *jsonEncoder) AddUint(k string, v uint) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AddUint32(k string, v uint32) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AddUint16(k string, v uint16) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AddUint8(k string, v uint8) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AddUintptr(k string, v uintptr) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AppendComplex64(v complex64) { enc.AppendComplex128(complex128(v)) }
func (enc *jsonEncoder) AppendFloat64(v float64) { enc.appendFloat(v, 64) }
func (enc *jsonEncoder) AppendFloat32(v float32) { enc.appendFloat(float64(v), 32) }
func (enc *jsonEncoder) AppendInt(v int) { enc.AppendInt64(int64(v)) }
func (enc *jsonEncoder) AppendInt32(v int32) { enc.AppendInt64(int64(v)) }
func (enc *jsonEncoder) AppendInt16(v int16) { enc.AppendInt64(int64(v)) }
func (enc *jsonEncoder) AppendInt8(v int8) { enc.AppendInt64(int64(v)) }
func (enc *jsonEncoder) AppendUint(v uint) { enc.AppendUint64(uint64(v)) }
func (enc *jsonEncoder) AppendUint32(v uint32) { enc.AppendUint64(uint64(v)) }
func (enc *jsonEncoder) AppendUint16(v uint16) { enc.AppendUint64(uint64(v)) }
func (enc *jsonEncoder) AppendUint8(v uint8) { enc.AppendUint64(uint64(v)) }
func (enc *jsonEncoder) AppendUintptr(v uintptr) { enc.AppendUint64(uint64(v)) }
func (enc *jsonEncoder) AddInt(k string, v int) { enc.AddInt64(k, int64(v)) }
func (enc *jsonEncoder) AddInt32(k string, v int32) { enc.AddInt64(k, int64(v)) }
func (enc *jsonEncoder) AddInt16(k string, v int16) { enc.AddInt64(k, int64(v)) }
func (enc *jsonEncoder) AddInt8(k string, v int8) { enc.AddInt64(k, int64(v)) }
func (enc *jsonEncoder) AddUint(k string, v uint) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AddUint32(k string, v uint32) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AddUint16(k string, v uint16) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AddUint8(k string, v uint8) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AddUintptr(k string, v uintptr) { enc.AddUint64(k, uint64(v)) }
func (enc *jsonEncoder) AppendComplex64(v complex64) { enc.appendComplex(complex128(v), 32) }
func (enc *jsonEncoder) AppendComplex128(v complex128) { enc.appendComplex(complex128(v), 64) }
func (enc *jsonEncoder) AppendFloat64(v float64) { enc.appendFloat(v, 64) }
func (enc *jsonEncoder) AppendFloat32(v float32) { enc.appendFloat(float64(v), 32) }
Copy link
Contributor

@sywhang sywhang Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I should've probably commented on this when float32 encoding was being changed... But I think there may be a chance we're taking a performance hit here because now the payloads are not all 64-bits anymore, whereas before everything was 64 bits, so we may be taking a hit on alignment (and therefore cache locality+fragmentation for buffers in the pool).

Have we done any benchmarks with these changes? Just to be clear, I think incorrect encoding is still a bigger issue than a perf hit, but at least we should understand what impact it had. The perf impact may not be significant after all :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair. I'll add a benchmark and results.

func (enc *jsonEncoder) AppendInt(v int) { enc.AppendInt64(int64(v)) }
func (enc *jsonEncoder) AppendInt32(v int32) { enc.AppendInt64(int64(v)) }
func (enc *jsonEncoder) AppendInt16(v int16) { enc.AppendInt64(int64(v)) }
func (enc *jsonEncoder) AppendInt8(v int8) { enc.AppendInt64(int64(v)) }
func (enc *jsonEncoder) AppendUint(v uint) { enc.AppendUint64(uint64(v)) }
func (enc *jsonEncoder) AppendUint32(v uint32) { enc.AppendUint64(uint64(v)) }
func (enc *jsonEncoder) AppendUint16(v uint16) { enc.AppendUint64(uint64(v)) }
func (enc *jsonEncoder) AppendUint8(v uint8) { enc.AppendUint64(uint64(v)) }
func (enc *jsonEncoder) AppendUintptr(v uintptr) { enc.AppendUint64(uint64(v)) }

func (enc *jsonEncoder) Clone() Encoder {
clone := enc.clone()
Expand Down
10 changes: 10 additions & 0 deletions zapcore/json_encoder_bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ func BenchmarkJSONLogMarshalerFunc(b *testing.B) {
}
}

func BenchmarkZapJSONFloat32AndComplex64(b *testing.B) {
b.RunParallel(func(pb *testing.PB) {
for pb.Next() {
enc := NewJSONEncoder(testEncoderConfig())
enc.AddFloat32("float32", 3.14)
enc.AddComplex64("complex64", 2.71+3.14i)
}
})
}

func BenchmarkZapJSON(b *testing.B) {
b.RunParallel(func(pb *testing.PB) {
for pb.Next() {
Expand Down
20 changes: 20 additions & 0 deletions zapcore/json_encoder_impl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,32 +121,52 @@ func TestJSONEncoderObjectFields(t *testing.T) {
{"byteString", `"k":""`, func(e Encoder) { e.AddByteString("k", []byte{}) }},
{"byteString", `"k":""`, func(e Encoder) { e.AddByteString("k", nil) }},
{"complex128", `"k":"1+2i"`, func(e Encoder) { e.AddComplex128("k", 1+2i) }},
{"complex128/negative_i", `"k":"1-2i"`, func(e Encoder) { e.AddComplex128("k", 1-2i) }},
{"complex64", `"k":"1+2i"`, func(e Encoder) { e.AddComplex64("k", 1+2i) }},
{"complex64/negative_i", `"k":"1-2i"`, func(e Encoder) { e.AddComplex64("k", 1-2i) }},
{"complex64", `"k":"2.71+3.14i"`, func(e Encoder) { e.AddComplex64("k", 2.71+3.14i) }},
{"duration", `"k":0.000000001`, func(e Encoder) { e.AddDuration("k", 1) }},
{"duration/negative", `"k":-0.000000001`, func(e Encoder) { e.AddDuration("k", -1) }},
{"float64", `"k":1`, func(e Encoder) { e.AddFloat64("k", 1.0) }},
{"float64", `"k":10000000000`, func(e Encoder) { e.AddFloat64("k", 1e10) }},
{"float64", `"k":"NaN"`, func(e Encoder) { e.AddFloat64("k", math.NaN()) }},
{"float64", `"k":"+Inf"`, func(e Encoder) { e.AddFloat64("k", math.Inf(1)) }},
{"float64", `"k":"-Inf"`, func(e Encoder) { e.AddFloat64("k", math.Inf(-1)) }},
{"float64/pi", `"k":3.141592653589793`, func(e Encoder) { e.AddFloat64("k", math.Pi) }},
{"float32", `"k":1`, func(e Encoder) { e.AddFloat32("k", 1.0) }},
{"float32", `"k":2.71`, func(e Encoder) { e.AddFloat32("k", 2.71) }},
{"float32", `"k":0.1`, func(e Encoder) { e.AddFloat32("k", 0.1) }},
{"float32", `"k":10000000000`, func(e Encoder) { e.AddFloat32("k", 1e10) }},
{"float32", `"k":"NaN"`, func(e Encoder) { e.AddFloat32("k", float32(math.NaN())) }},
{"float32", `"k":"+Inf"`, func(e Encoder) { e.AddFloat32("k", float32(math.Inf(1))) }},
{"float32", `"k":"-Inf"`, func(e Encoder) { e.AddFloat32("k", float32(math.Inf(-1))) }},
{"float32/pi", `"k":3.1415927`, func(e Encoder) { e.AddFloat32("k", math.Pi) }},
{"int", `"k":42`, func(e Encoder) { e.AddInt("k", 42) }},
{"int64", `"k":42`, func(e Encoder) { e.AddInt64("k", 42) }},
{"int64/min", `"k":-9223372036854775808`, func(e Encoder) { e.AddInt64("k", math.MinInt64) }},
{"int64/max", `"k":9223372036854775807`, func(e Encoder) { e.AddInt64("k", math.MaxInt64) }},
{"int32", `"k":42`, func(e Encoder) { e.AddInt32("k", 42) }},
{"int32/min", `"k":-2147483648`, func(e Encoder) { e.AddInt32("k", math.MinInt32) }},
{"int32/max", `"k":2147483647`, func(e Encoder) { e.AddInt32("k", math.MaxInt32) }},
{"int16", `"k":42`, func(e Encoder) { e.AddInt16("k", 42) }},
{"int16/min", `"k":-32768`, func(e Encoder) { e.AddInt16("k", math.MinInt16) }},
{"int16/max", `"k":32767`, func(e Encoder) { e.AddInt16("k", math.MaxInt16) }},
{"int8", `"k":42`, func(e Encoder) { e.AddInt8("k", 42) }},
{"int8/min", `"k":-128`, func(e Encoder) { e.AddInt8("k", math.MinInt8) }},
{"int8/max", `"k":127`, func(e Encoder) { e.AddInt8("k", math.MaxInt8) }},
{"string", `"k":"v\\"`, func(e Encoder) { e.AddString(`k`, `v\`) }},
{"string", `"k":"v"`, func(e Encoder) { e.AddString("k", "v") }},
{"string", `"k":""`, func(e Encoder) { e.AddString("k", "") }},
{"time", `"k":1`, func(e Encoder) { e.AddTime("k", time.Unix(1, 0)) }},
{"uint", `"k":42`, func(e Encoder) { e.AddUint("k", 42) }},
{"uint64", `"k":42`, func(e Encoder) { e.AddUint64("k", 42) }},
{"uint64/max", `"k":18446744073709551615`, func(e Encoder) { e.AddUint64("k", math.MaxUint64) }},
{"uint32", `"k":42`, func(e Encoder) { e.AddUint32("k", 42) }},
{"uint32/max", `"k":4294967295`, func(e Encoder) { e.AddUint32("k", math.MaxUint32) }},
{"uint16", `"k":42`, func(e Encoder) { e.AddUint16("k", 42) }},
{"uint16/max", `"k":65535`, func(e Encoder) { e.AddUint16("k", math.MaxUint16) }},
{"uint8", `"k":42`, func(e Encoder) { e.AddUint8("k", 42) }},
{"uint8/max", `"k":255`, func(e Encoder) { e.AddUint8("k", math.MaxUint8) }},
{"uintptr", `"k":42`, func(e Encoder) { e.AddUintptr("k", 42) }},
{
desc: "object (success)",
Expand Down