JSON: Fix complex64 encoding precision #1011
Conversation
Per #1010, #1002, and #995, some of the corner cases where precision is changed or lost aren't fully tested. Add test cases for corner cases for a number of these: - complex{64, 128}: Test incorrect precision and negatives - float{32, 64}: Test incorrect precision - int{8, 16, 32, 64}: Test minimum and maximum values - uint{8, 16, 32, 64}: Test maximum values Per #1010, the test for complex64 incorrect precision is currently failing.
Converting to Complex128 meant that the real and imaginary components of the Complex64 were encoded as float64s which, like #1002, ended up those components with the incorrect precision. Fix this by adding a precision parameter to appendComplex tp specify the precision with which the two components of the complex number should be encoded.
Codecov Report
@@ Coverage Diff @@
## master #1011 +/- ##
==========================================
+ Coverage 98.10% 98.20% +0.09%
==========================================
Files 46 46
Lines 2058 2060 +2
==========================================
+ Hits 2019 2023 +4
+ Misses 30 29 -1
+ Partials 9 8 -1
Continue to review full report at Codecov.
|
| 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) } |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Fair. I'll add a benchmark and results.
Converting
complex64tocomplex128meant thatthe real and imaginary components of the
complex64valuewere encoded as
float64values which,similar to #1002, encoded them with incorrect precision.
Fix this by adding a precision parameter during encoding to specify
the precision with which the components should be encoded.
Guard against other such cases where there's loss of precision
by adding tests for several such corner cases for
complex, float, int, and uint.
This has no significant performance impact on my laptop.
(The drop is likely a false positive.)
Fixes #1010
Ref GO-872