GODRIVER-1808 Fix BSON unmarshaling into an interface containing a concrete value. #1584
Conversation
| return nil, err | ||
| var vDecoder *ValueDecoder | ||
| var tDecoder *typeDecoder | ||
| if !(eType.Kind() == reflect.Interface && val.Len() > 0) { |
There was a problem hiding this comment.
Delay the decoder retrieval for interface slice/array so each element will be decoded based on its type.
API Change Report./bsonincompatible changes##(*Decoder).Reset: changed from func(ValueReader) to func(./bson/bsonrw.ValueReader) compatible changesRaw.Elements: added ./bson/bsoncodeccompatible changespackage added ./bson/bsonrwcompatible changespackage added ./bson/bsonrw/bsonrwtestcompatible changespackage added ./bson/bsontypecompatible changespackage added ./bson/mgocompatincompatible changes##GetterEncodeValue: changed from func(./bson.EncodeContext, ./bson.ValueWriter, reflect.Value) error to func(./bson/bsoncodec.EncodeContext, ./bson/bsonrw.ValueWriter, reflect.Value) error compatible changesRegistryRespectNilValues: added ./bson/primitivecompatible changespackage added ./eventincompatible changes##CommandFailedEvent.ServiceID: changed from *./bson.ObjectID to *./bson/primitive.ObjectID ./mongoincompatible changes(*ChangeStream).RemainingBatchLength: removed compatible changesSessionContext: added ./mongo/descriptionincompatible changes##SelectedServer.ElectionID: changed from ./bson.ObjectID to ./bson/primitive.ObjectID ./mongo/optionsincompatible changes##(*AggregateOptions).SetCustom: changed from func(./bson.M) *AggregateOptions to func(./bson/primitive.M) *AggregateOptions ./mongo/readconcernincompatible changes##(*ReadConcern).MarshalBSONValue: changed from func() (./bson.Type, []byte, error) to func() (./bson/bsontype.Type, []byte, error) ./mongo/writeconcernincompatible changes##(*WriteConcern).MarshalBSONValue: changed from func() (./bson.Type, []byte, error) to func() (./bson/bsontype.Type, []byte, error) ./x/bsonx/bsoncoreincompatible changes##(*ArrayBuilder).AppendDBPointer: changed from func(string, [12]byte) *ArrayBuilder to func(string, ./bson/primitive.ObjectID) *ArrayBuilder ./x/mongo/driverincompatible changes##CursorResponse.Connection: changed from *./x/mongo/driver/mnet.Connection to PinnedConnection compatible changesCompressor: added ./x/mongo/driver/authincompatible changes##Config.Connection: changed from *./x/mongo/driver/mnet.Connection to ./x/mongo/driver.Connection compatible changesConfig.Description: added ./x/mongo/driver/drivertestincompatible changes(*ChannelConn).Read: removed compatible changes(*ChannelConn).ReadWireMessage: added ./x/mongo/driver/mnetincompatible changespackage removed ./x/mongo/driver/mongocrypt/optionsincompatible changes##(*ExplicitEncryptionOptions).SetKeyID: changed from func(./bson.Binary) *ExplicitEncryptionOptions to func(./bson/primitive.Binary) *ExplicitEncryptionOptions ./x/mongo/driver/operationincompatible changes##(*Hello).FinishHandshake: changed from func(context.Context, *./x/mongo/driver/mnet.Connection) error to func(context.Context, ./x/mongo/driver.Connection) error compatible changes(*CreateSearchIndexes).WriteConcern: added ./x/mongo/driver/sessionincompatible changes##(Client).AdvanceOperationTime: changed from func(./bson.Timestamp) error to func(*./bson/primitive.Timestamp) error ./x/mongo/driver/topologyincompatible changes(*Connection).Read: removed compatible changes(*Connection).ReadWireMessage: added |
qingyang-hu
force-pushed
the
godriver1808
branch
from
60820b9
to
1db6753
Compare
qingyang-hu
changed the title
There was a problem hiding this comment.
We should update the implementation to remove the added calls to reflect.New, which seems to increase memory allocs in some cases. Is there a way to pass a reflect.Type when we only have type information, and pass a reflect.Value when we have value and type information?
I noticed BenchmarkUnmarshal reports about 30% more allocations with this PR. Keep in mind that BenchmarkUnmarshal actually only benchmarks unmarhshaling into a map. BenchmarkCodeUnmarshal, which unmarshals into a struct, shows no difference, so it seems to be isolated to map unmarshaling.
❯ benchstat old.txt new.txt
goos: darwin
goarch: arm64
pkg: go.mongodb.org/mongo-driver/bson
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
Unmarshal/simple_struct/BSON-10 3.423µ ± 0% 3.961µ ± 19% +15.72% (p=0.000 n=10)
Unmarshal/nested_struct/BSON-10 8.605µ ± 1% 9.258µ ± 0% +7.59% (p=0.000 n=10)
Unmarshal/deep_bson.json.gz/BSON-10 41.98µ ± 0% 46.45µ ± 1% +10.66% (p=0.000 n=10)
Unmarshal/flat_bson.json.gz/BSON-10 41.88µ ± 1% 47.73µ ± 0% +13.97% (p=0.000 n=10)
Unmarshal/full_bson.json.gz/BSON-10 49.61µ ± 0% 55.90µ ± 0% +12.67% (p=0.000 n=10)
geomean 19.14µ 21.45µ +12.09%
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
Unmarshal/simple_struct/BSON-10 1.062Ki ± 0% 1.375Ki ± 0% +29.41% (p=0.000 n=10)
Unmarshal/nested_struct/BSON-10 7.927Ki ± 0% 8.411Ki ± 0% +6.11% (p=0.000 n=10)
Unmarshal/deep_bson.json.gz/BSON-10 30.15Ki ± 0% 33.12Ki ± 0% +9.85% (p=0.000 n=10)
Unmarshal/flat_bson.json.gz/BSON-10 13.11Ki ± 0% 16.79Ki ± 0% +28.09% (p=0.000 n=10)
Unmarshal/full_bson.json.gz/BSON-10 19.93Ki ± 0% 23.57Ki ± 0% +18.27% (p=0.000 n=10)
geomean 9.212Ki 10.87Ki +17.97%
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
Unmarshal/simple_struct/BSON-10 62.00 ± 0% 86.00 ± 0% +38.71% (p=0.000 n=10)
Unmarshal/nested_struct/BSON-10 143.0 ± 0% 178.0 ± 0% +24.48% (p=0.000 n=10)
Unmarshal/deep_bson.json.gz/BSON-10 822.0 ± 0% 1012.0 ± 0% +23.11% (p=0.000 n=10)
Unmarshal/flat_bson.json.gz/BSON-10 740.0 ± 0% 1030.0 ± 0% +39.19% (p=0.000 n=10)
Unmarshal/full_bson.json.gz/BSON-10 797.0 ± 0% 1086.0 ± 0% +36.26% (p=0.000 n=10)
geomean 336.2 444.4 +32.16%
Here's a graph view of a memory profile of running BenchmarkUnmarshal on the previous commit, filtered to only reflect calls:
And here's the same profile of the PR commit, filtered to only reflect calls:
The allocs from reflect.New grew from 0.28GB to 1GB, which seems to be the biggest change between the two profiles.
bson/bsoncodec/bsoncodec.go
Outdated
| } | ||
|
|
||
| func decodeTypeOrValueWithInfo(vd ValueDecoder, td typeDecoder, dc DecodeContext, vr bsonrw.ValueReader, t reflect.Type, convert bool) (reflect.Value, error) { | ||
| func decodeTypeOrValueWithInfo(vd ValueDecoder, td typeDecoder, dc DecodeContext, vr bsonrw.ValueReader, val reflect.Value, convert bool) (reflect.Value, error) { |
There was a problem hiding this comment.
We should change the reflect.Value parameter back to reflect.Type because all we do with that reflect.Value is extract the type. Instead, we should pass the reflect.Type to avoid the unnecessary reflect.New allocations.
| val.Index(idx).Set(v) | ||
| e = v | ||
| } | ||
| elem, err = decodeTypeOrValueWithInfo(valueDecoder, typeDecoder, dc, vr, e, true) |
There was a problem hiding this comment.
We should call e.Type() here instead of passing the reflect.Value.
| elem, err = decodeTypeOrValueWithInfo(valueDecoder, typeDecoder, dc, vr, e, true) | |
| elem, err = decodeTypeOrValueWithInfo(valueDecoder, typeDecoder, dc, vr, e.Type(), true) |
| elem = reflect.Zero(val.Index(idx).Type()) | ||
| } | ||
| } else { | ||
| elem, err = decodeTypeOrValueWithInfo(vDecoder, tDecoder, dc, vr, reflect.New(eType).Elem(), true) |
There was a problem hiding this comment.
We should pass the type here instead of allocating a new reflect.Value.
| elem, err = decodeTypeOrValueWithInfo(vDecoder, tDecoder, dc, vr, reflect.New(eType).Elem(), true) | |
| elem, err = decodeTypeOrValueWithInfo(vDecoder, tDecoder, dc, vr, eType, true) |
bson/bsoncodec/map_codec.go
Outdated
| @@ -213,7 +213,7 @@ func (mc *MapCodec) DecodeValue(dc DecodeContext, vr bsonrw.ValueReader, val ref | |||
| return err | |||
| } | |||
|
|
|||
| elem, err := decodeTypeOrValueWithInfo(decoder, eTypeDecoder, dc, vr, eType, true) | |||
| elem, err := decodeTypeOrValueWithInfo(decoder, eTypeDecoder, dc, vr, reflect.New(eType).Elem(), true) | |||
There was a problem hiding this comment.
We should pass the type here instead of allocating a new reflect.Value.
| elem, err := decodeTypeOrValueWithInfo(decoder, eTypeDecoder, dc, vr, reflect.New(eType).Elem(), true) | |
| elem, err := decodeTypeOrValueWithInfo(decoder, eTypeDecoder, dc, vr, eType, true) |
bson/bsoncodec/bsoncodec.go
Outdated
| val := reflect.New(t).Elem() | ||
| return decodeTypeOrValueWithInfo(decoder, td, dc, vr, val, true) |
There was a problem hiding this comment.
We should pass the type here instead of allocating a new reflect.Value.
| val := reflect.New(t).Elem() | |
| return decodeTypeOrValueWithInfo(decoder, td, dc, vr, val, true) | |
| return decodeTypeOrValueWithInfo(decoder, td, dc, vr, t, true) |
| @@ -185,7 +185,7 @@ func (dvd DefaultValueDecoders) DDecodeValue(dc DecodeContext, vr bsonrw.ValueRe | |||
| } | |||
|
|
|||
| // Pass false for convert because we don't need to call reflect.Value.Convert for tEmpty. | |||
| elem, err := decodeTypeOrValueWithInfo(decoder, tEmptyTypeDecoder, dc, elemVr, tEmpty, false) | |||
| elem, err := decodeTypeOrValueWithInfo(decoder, tEmptyTypeDecoder, dc, elemVr, reflect.New(tEmpty).Elem(), false) | |||
There was a problem hiding this comment.
We should pass the type here instead of allocating a new reflect.Value.
| elem, err := decodeTypeOrValueWithInfo(decoder, tEmptyTypeDecoder, dc, elemVr, reflect.New(tEmpty).Elem(), false) | |
| elem, err := decodeTypeOrValueWithInfo(decoder, tEmptyTypeDecoder, dc, elemVr, tEmpty, false) |
GODRIVER-1808
Summary
This PR fixes BSON's unmarshaling behavior for interfaces by passing
reflect.Valueinstead ofreflect.Type, so the actual value type contained in the interface can be used.Especially for an interface containing a pointer, this PR unmarshals value into the pointed concrete value.
Background & Motivation
This PR provides a more flexible way of unmarshaling and makes the behavior closer to the
JSONpackage.