Skip to content

Commit

Permalink
codec: streamline nil decoding: nil means zero value consistently
Browse files Browse the repository at this point in the history
Previously, we had a TryDecodeAsNil that all formats implemented.

However, the same formats did double duty by still checking if nil in stream
when decoding a value.

We introduce TryNil (replacing TryDecodeAsNil) which is used iff
the type is a pointer and we need to set it to null if null was seen in the stream.

Now, nil means zero value consistently, regardless of the context.
In other words, a 'nil' in a stream means to reset the value to its zero state.

A side effect of this is that the decode option 'DeleteOnNilMapValue`
is a no-op. It is kept for compiling compatibility but has no effect
during decoding.

The main benefits are in the architecture
- TryNil is only called if updating a pointer value
- Nil is only handled at container boundaries: map, slice, null
  i.e. only when calling decDriver.ContainerType or decDriver.Read(Map|Array)Len

----

Also, reduce size of typeInfo by using a bitset flag instead of multiple bools.

This gives us space (within 3*8 words) to store a reflect.Zero value
in each typeInfo. This saves some allocation done when calling reflect.Zero(Type).

----

Add some code cleanup and re-organization.
  • Loading branch information
ugorji committed Jun 12, 2019
1 parent a2a200a commit ffda9d4
Show file tree
Hide file tree
Showing 26 changed files with 6,159 additions and 8,151 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ some caveats. See Encode documentation.

```go
const CborStreamBytes byte = 0x5f ...
const GenVersion = 13
const GenVersion = 14
var SelfExt = &extFailWrapper{}
var GoRpc goRpc
var MsgpackSpecRpc msgpackSpecRpc
Expand Down
32 changes: 20 additions & 12 deletions codec/bench/shared_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,12 +276,13 @@ func sTestCodecDecode(bs []byte, ts interface{}, h Handle, bh *BasicHandle) (err
// These are for intormational messages that do not necessarily
// help with diagnosing a failure, or which are too large.
func logTv(x interface{}, format string, args ...interface{}) {
if testVerbose {
if t, ok := x.(testing.TB); ok { // only available from go 1.9
t.Helper()
}
logT(x, format, args...)
if !testVerbose {
return
}
if t, ok := x.(testing.TB); ok { // only available from go 1.9
t.Helper()
}
logT(x, format, args...)
}

// logT logs messages when running as go test -v
Expand All @@ -304,12 +305,22 @@ func logT(x interface{}, format string, args ...interface{}) {
}
}

func failT(x interface{}, args ...interface{}) {
t, ok := x.(testing.TB) // only available from go 1.9
if ok {
t.Helper()
func failTv(x testing.TB, args ...interface{}) {
x.Helper()
if testVerbose {
failTMsg(x, args...)
}
x.FailNow()
}

func failT(x testing.TB, args ...interface{}) {
x.Helper()
failTMsg(x, args...)
x.FailNow()
}

func failTMsg(x testing.TB, args ...interface{}) {
x.Helper()
if len(args) > 0 {
if format, ok := args[0].(string); ok {
logT(x, format, args[1:]...)
Expand All @@ -319,9 +330,6 @@ func failT(x interface{}, args ...interface{}) {
logT(x, "%v", args)
}
}
if ok {
t.FailNow()
}
}

// --- functions below are used only by benchmarks alone
Expand Down
103 changes: 56 additions & 47 deletions codec/binc.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,8 @@ type bincDecDriver struct {
bd byte
vd byte
vs byte

fnil bool
// _ [3]byte // padding
// linear searching on this slice is ok,
// because we typically expect < 32 symbols in each stream.
Expand All @@ -419,11 +421,36 @@ func (d *bincDecDriver) uncacheRead() {
}
}

func (d *bincDecDriver) advanceNil() (null bool) {
d.fnil = false
if !d.bdRead {
d.readNextBd()
}
if d.bd == bincVdSpecial<<4|bincSpNil {
d.bdRead = false
d.fnil = true
null = true
}
return
}

func (d *bincDecDriver) Nil() bool {
return d.fnil
}

func (d *bincDecDriver) TryNil() bool {
return d.advanceNil()
}

func (d *bincDecDriver) ContainerType() (vt valueType) {
if !d.bdRead {
d.readNextBd()
}
if d.vd == bincVdSpecial && d.vs == bincSpNil {
d.fnil = false
// if d.vd == bincVdSpecial && d.vs == bincSpNil {
if d.bd == bincVdSpecial<<4|bincSpNil {
d.bdRead = false
d.fnil = true
return valueTypeNil
} else if d.vd == bincVdByteArray {
return valueTypeBytes
Expand All @@ -440,23 +467,8 @@ func (d *bincDecDriver) ContainerType() (vt valueType) {
return valueTypeUnset
}

func (d *bincDecDriver) TryDecodeAsNil() bool {
if !d.bdRead {
d.readNextBd()
}
if d.bd == bincVdSpecial<<4|bincSpNil {
d.bdRead = false
return true
}
return false
}

func (d *bincDecDriver) DecodeTime() (t time.Time) {
if !d.bdRead {
d.readNextBd()
}
if d.bd == bincVdSpecial<<4|bincSpNil {
d.bdRead = false
if d.advanceNil() {
return
}
if d.vd != bincVdTimestamp {
Expand Down Expand Up @@ -536,9 +548,6 @@ func (d *bincDecDriver) decUint() (v uint64) {
}

func (d *bincDecDriver) decCheckInteger() (ui uint64, neg bool) {
if !d.bdRead {
d.readNextBd()
}
vd, vs := d.vd, d.vs
if vd == bincVdPosInt {
ui = d.decUint()
Expand Down Expand Up @@ -566,6 +575,9 @@ func (d *bincDecDriver) decCheckInteger() (ui uint64, neg bool) {
}

func (d *bincDecDriver) DecodeInt64() (i int64) {
if d.advanceNil() {
return
}
ui, neg := d.decCheckInteger()
i = chkOvf.SignedIntV(ui)
if neg {
Expand All @@ -576,6 +588,9 @@ func (d *bincDecDriver) DecodeInt64() (i int64) {
}

func (d *bincDecDriver) DecodeUint64() (ui uint64) {
if d.advanceNil() {
return
}
ui, neg := d.decCheckInteger()
if neg {
d.d.errorf("assigning negative signed value to unsigned integer type")
Expand All @@ -586,8 +601,8 @@ func (d *bincDecDriver) DecodeUint64() (ui uint64) {
}

func (d *bincDecDriver) DecodeFloat64() (f float64) {
if !d.bdRead {
d.readNextBd()
if d.advanceNil() {
return
}
vd, vs := d.vd, d.vs
if vd == bincVdSpecial {
Expand Down Expand Up @@ -616,12 +631,12 @@ func (d *bincDecDriver) DecodeFloat64() (f float64) {

// bool can be decoded from bool only (single byte).
func (d *bincDecDriver) DecodeBool() (b bool) {
if !d.bdRead {
d.readNextBd()
if d.advanceNil() {
return
}
if bd := d.bd; bd == (bincVdSpecial | bincSpFalse) {
if d.bd == (bincVdSpecial | bincSpFalse) {
// b = false
} else if bd == (bincVdSpecial | bincSpTrue) {
} else if d.bd == (bincVdSpecial | bincSpTrue) {
b = true
} else {
d.d.errorf("bool - %s %x-%x/%s", msgBadDesc, d.vd, d.vs, bincdesc(d.vd, d.vs))
Expand All @@ -632,8 +647,8 @@ func (d *bincDecDriver) DecodeBool() (b bool) {
}

func (d *bincDecDriver) ReadMapStart() (length int) {
if !d.bdRead {
d.readNextBd()
if d.advanceNil() {
return decContainerLenNil
}
if d.vd != bincVdMap {
d.d.errorf("map - %s %x-%x/%s", msgBadDesc, d.vd, d.vs, bincdesc(d.vd, d.vs))
Expand All @@ -645,8 +660,8 @@ func (d *bincDecDriver) ReadMapStart() (length int) {
}

func (d *bincDecDriver) ReadArrayStart() (length int) {
if !d.bdRead {
d.readNextBd()
if d.advanceNil() {
return decContainerLenNil
}
if d.vd != bincVdArray {
d.d.errorf("array - %s %x-%x/%s", msgBadDesc, d.vd, d.vs, bincdesc(d.vd, d.vs))
Expand Down Expand Up @@ -681,11 +696,7 @@ func (d *bincDecDriver) decLenNumber() (v uint64) {
}

func (d *bincDecDriver) decStringBytes(bs []byte, zerocopy bool) (bs2 []byte) {
if !d.bdRead {
d.readNextBd()
}
if d.bd == bincVdSpecial<<4|bincSpNil {
d.bdRead = false
if d.advanceNil() {
return
}
var slen = -1
Expand Down Expand Up @@ -756,12 +767,8 @@ func (d *bincDecDriver) DecodeStringAsBytes() (s []byte) {
}

func (d *bincDecDriver) DecodeBytes(bs []byte, zerocopy bool) (bsOut []byte) {
if !d.bdRead {
d.readNextBd()
}
if d.bd == bincVdSpecial<<4|bincSpNil {
d.bdRead = false
return nil
if d.advanceNil() {
return
}
// check if an "array" of uint8's (see ContainerType for how to infer if an array)
if d.vd == bincVdArray {
Expand Down Expand Up @@ -794,13 +801,16 @@ func (d *bincDecDriver) DecodeBytes(bs []byte, zerocopy bool) (bsOut []byte) {
return decByteSlice(d.r, clen, d.d.h.MaxInitLen, bs)
}

func (d *bincDecDriver) DecodeExt(rv interface{}, xtag uint64, ext Ext) (realxtag uint64) {
func (d *bincDecDriver) DecodeExt(rv interface{}, xtag uint64, ext Ext) {
if xtag > 0xff {
d.d.errorf("ext: tag must be <= 0xff; got: %v", xtag)
return
}
if d.advanceNil() {
return
}
realxtag1, xbs := d.decodeExtV(ext != nil, uint8(xtag))
realxtag = uint64(realxtag1)
realxtag := uint64(realxtag1)
if ext == nil {
re := rv.(*RawExt)
re.Tag = realxtag
Expand All @@ -810,13 +820,9 @@ func (d *bincDecDriver) DecodeExt(rv interface{}, xtag uint64, ext Ext) (realxta
} else {
ext.ReadExt(rv, xbs)
}
return
}

func (d *bincDecDriver) decodeExtV(verifyTag bool, tag byte) (xtag byte, xbs []byte) {
if !d.bdRead {
d.readNextBd()
}
if d.vd == bincVdCustomExt {
l := d.decLen()
xtag = d.r.readn1()
Expand Down Expand Up @@ -845,6 +851,7 @@ func (d *bincDecDriver) DecodeNaked() {
d.readNextBd()
}

d.fnil = false
n := d.d.naked()
var decodeFurther bool

Expand All @@ -853,6 +860,7 @@ func (d *bincDecDriver) DecodeNaked() {
switch d.vs {
case bincSpNil:
n.v = valueTypeNil
d.fnil = true
case bincSpFalse:
n.v = valueTypeBool
n.b = false
Expand Down Expand Up @@ -1014,6 +1022,7 @@ func (d *bincDecDriver) reset() {
d.r, d.br = d.d.r(), d.d.bytes
d.s = nil
d.bd, d.bdRead, d.vd, d.vs = 0, false, 0, 0
d.fnil = false
}

func (d *bincDecDriver) atEndOfDecode() {
Expand Down
Loading

0 comments on commit ffda9d4

Please sign in to comment.