GODRIVER-1808 Fix BSON unmarshaling into an interface containing a concrete value. #1584

qingyang-hu · 2024-03-20T18:31:20Z

Summary

This PR fixes BSON's unmarshaling behavior for interfaces by passing reflect.Value instead of reflect.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 JSON package.

qingyang-hu · 2024-03-20T18:33:36Z

bson/bsoncodec/default_value_decoders.go

-		return nil, err
+	var vDecoder *ValueDecoder
+	var tDecoder *typeDecoder
+	if !(eType.Kind() == reflect.Interface && val.Len() > 0) {


Delay the decoder retrieval for interface slice/array so each element will be decoded based on its type.

mongodb-drivers-pr-bot · 2024-03-20T18:50:06Z

API Change Report

./x/mongo/driver

incompatible changes

##CursorResponse.Connection: changed from *./x/mongo/driver/mnet.Connection to PinnedConnection
##ErrorProcessor.ProcessError: changed from func(error, ./x/mongo/driver/mnet.Describer) ProcessErrorResult to func(error, Connection) ProcessErrorResult
##Handshaker.FinishHandshake: changed from func(context.Context, *./x/mongo/driver/mnet.Connection) error to func(context.Context, Connection) error
##Handshaker.GetHandshakeInformation: changed from func(context.Context, ./mongo/address.Address, *./x/mongo/driver/mnet.Connection) (HandshakeInformation, error) to func(context.Context, ./mongo/address.Address, Connection) (HandshakeInformation, error)
##Operation.ExecuteExhaust: changed from func(context.Context, *./x/mongo/driver/mnet.Connection) error to func(context.Context, StreamerConnection) error
##ResponseInfo.Connection: changed from ./x/mongo/driver/mnet.Connection to Connection
##Server.Connection: changed from func(context.Context) (./x/mongo/driver/mnet.Connection, error) to func(context.Context) (Connection, error)
##SingleConnectionDeployment.C: changed from ./x/mongo/driver/mnet.Connection to Connection
##SingleConnectionDeployment.Connection: changed from func(context.Context) (./x/mongo/driver/mnet.Connection, error) to func(context.Context) (Connection, error)

compatible changes

Compressor: added
Connection: added
PinnedConnection: added
StreamerConnection: added

./x/mongo/driver/auth

incompatible changes

##Config.Connection: changed from *./x/mongo/driver/mnet.Connection to ./x/mongo/driver.Connection

compatible changes

Config.Description: added

./x/mongo/driver/drivertest

incompatible changes

(*ChannelConn).Read: removed
(*ChannelConn).Write: removed

compatible changes

(*ChannelConn).ReadWireMessage: added
(*ChannelConn).WriteWireMessage: added

./x/mongo/driver/mnet

incompatible changes

package removed

./x/mongo/driver/operation

incompatible changes

##(*Hello).FinishHandshake: changed from func(context.Context, *./x/mongo/driver/mnet.Connection) error to func(context.Context, ./x/mongo/driver.Connection) error
##(*Hello).GetHandshakeInformation: changed from func(context.Context, ./mongo/address.Address, *./x/mongo/driver/mnet.Connection) (./x/mongo/driver.HandshakeInformation, error) to func(context.Context, ./mongo/address.Address, ./x/mongo/driver.Connection) (./x/mongo/driver.HandshakeInformation, error)
##(*Hello).StreamResponse: changed from func(context.Context, *./x/mongo/driver/mnet.Connection) error to func(context.Context, ./x/mongo/driver.StreamerConnection) error

./x/mongo/driver/session

incompatible changes

LoadBalancedTransactionConnection.ReadWireMessage: added
LoadBalancedTransactionConnection.WriteWireMessage: added
##./x/mongo/driver/mnet.ReadWriteCloser.Read, method set of LoadBalancedTransactionConnection: removed
##./x/mongo/driver/mnet.ReadWriteCloser.Write, method set of LoadBalancedTransactionConnection: removed

./x/mongo/driver/topology

incompatible changes

(*Connection).Read: removed
(*Connection).Write: removed
##(Server).Connection: changed from func(context.Context) (./x/mongo/driver/mnet.Connection, error) to func(context.Context) (./x/mongo/driver.Connection, error)
##(*Server).ProcessError: changed from func(error, ./x/mongo/driver/mnet.Describer) ./x/mongo/driver.ProcessErrorResult to func(error, ./x/mongo/driver.Connection) ./x/mongo/driver.ProcessErrorResult

compatible changes

(*Connection).ReadWireMessage: added
(*Connection).WriteWireMessage: added

…te values.

matthewdale

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.

matthewdale

Investigating this further, there are a few quick changes we can make to keep the allocations identical to the base branch.

matthewdale · 2024-04-26T16:53:17Z

bson/bsoncodec/bsoncodec.go

 }

-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) {


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.

matthewdale · 2024-04-26T16:54:59Z

bson/bsoncodec/default_value_decoders.go

+				val.Index(idx).Set(v)
+				e = v
+			}
+			elem, err = decodeTypeOrValueWithInfo(valueDecoder, typeDecoder, dc, vr, e, true)


We should call e.Type() here instead of passing the reflect.Value.

Suggested change

elem, err = decodeTypeOrValueWithInfo(valueDecoder, typeDecoder, dc, vr, e, true)

elem, err = decodeTypeOrValueWithInfo(valueDecoder, typeDecoder, dc, vr, e.Type(), true)

matthewdale · 2024-04-26T16:56:07Z

bson/bsoncodec/default_value_decoders.go

+				elem = reflect.Zero(val.Index(idx).Type())
+			}
+		} else {
+			elem, err = decodeTypeOrValueWithInfo(vDecoder, tDecoder, dc, vr, reflect.New(eType).Elem(), true)


We should pass the type here instead of allocating a new reflect.Value.

Suggested change

elem, err = decodeTypeOrValueWithInfo(vDecoder, tDecoder, dc, vr, reflect.New(eType).Elem(), true)

elem, err = decodeTypeOrValueWithInfo(vDecoder, tDecoder, dc, vr, eType, true)

matthewdale · 2024-04-26T16:56:38Z

bson/bsoncodec/map_codec.go

@@ -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)


We should pass the type here instead of allocating a new reflect.Value.

Suggested change

elem, err := decodeTypeOrValueWithInfo(decoder, eTypeDecoder, dc, vr, reflect.New(eType).Elem(), true)

elem, err := decodeTypeOrValueWithInfo(decoder, eTypeDecoder, dc, vr, eType, true)

matthewdale · 2024-04-26T16:57:06Z

bson/bsoncodec/bsoncodec.go

+	val := reflect.New(t).Elem()
+	return decodeTypeOrValueWithInfo(decoder, td, dc, vr, val, true)


We should pass the type here instead of allocating a new reflect.Value.

Suggested change

val := reflect.New(t).Elem()

return decodeTypeOrValueWithInfo(decoder, td, dc, vr, val, true)

return decodeTypeOrValueWithInfo(decoder, td, dc, vr, t, true)

matthewdale · 2024-04-26T16:57:31Z

bson/bsoncodec/default_value_decoders.go

@@ -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)


We should pass the type here instead of allocating a new reflect.Value.

Suggested change

elem, err := decodeTypeOrValueWithInfo(decoder, tEmptyTypeDecoder, dc, elemVr, reflect.New(tEmpty).Elem(), false)

elem, err := decodeTypeOrValueWithInfo(decoder, tEmptyTypeDecoder, dc, elemVr, tEmpty, false)

prestonvasquez

LGTM, with updates proposed by Matt.

qingyang-hu commented Mar 20, 2024

View reviewed changes

mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Mar 20, 2024

GODRIVER-1808 Fix unmarshaling BSON into interfaces containing concre…

1db6753

…te values.

qingyang-hu force-pushed the godriver1808 branch from 60820b9 to 1db6753 Compare March 20, 2024 21:22

qingyang-hu marked this pull request as ready for review March 20, 2024 22:16

qingyang-hu requested review from matthewdale and prestonvasquez March 20, 2024 22:18

qingyang-hu changed the title ~~GODRIVER-1808~~ GODRIVER-1808 Fix BSON unmarshaling into an interface containing a concrete value. Mar 21, 2024

matthewdale requested changes Apr 20, 2024

View reviewed changes

qingyang-hu marked this pull request as draft April 22, 2024 16:29

matthewdale requested changes Apr 26, 2024

View reviewed changes

matthewdale mentioned this pull request May 2, 2024

GODRIVER-2443 Make Distinct return a decodable struct #1603

Merged

prestonvasquez approved these changes May 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-1808 Fix BSON unmarshaling into an interface containing a concrete value. #1584

GODRIVER-1808 Fix BSON unmarshaling into an interface containing a concrete value. #1584

qingyang-hu commented Mar 20, 2024

qingyang-hu Mar 20, 2024

mongodb-drivers-pr-bot bot commented Mar 20, 2024

matthewdale left a comment •

edited

matthewdale left a comment

matthewdale Apr 26, 2024

matthewdale Apr 26, 2024

matthewdale Apr 26, 2024

matthewdale Apr 26, 2024

matthewdale Apr 26, 2024

matthewdale Apr 26, 2024

prestonvasquez left a comment

	elem, err = decodeTypeOrValueWithInfo(valueDecoder, typeDecoder, dc, vr, e, true)
	elem, err = decodeTypeOrValueWithInfo(valueDecoder, typeDecoder, dc, vr, e.Type(), true)

	elem, err = decodeTypeOrValueWithInfo(vDecoder, tDecoder, dc, vr, reflect.New(eType).Elem(), true)
	elem, err = decodeTypeOrValueWithInfo(vDecoder, tDecoder, dc, vr, eType, true)

	elem, err := decodeTypeOrValueWithInfo(decoder, eTypeDecoder, dc, vr, reflect.New(eType).Elem(), true)
	elem, err := decodeTypeOrValueWithInfo(decoder, eTypeDecoder, dc, vr, eType, true)

		val := reflect.New(t).Elem()
		return decodeTypeOrValueWithInfo(decoder, td, dc, vr, val, true)

GODRIVER-1808 Fix BSON unmarshaling into an interface containing a concrete value. #1584

Are you sure you want to change the base?

GODRIVER-1808 Fix BSON unmarshaling into an interface containing a concrete value. #1584

Conversation

qingyang-hu commented Mar 20, 2024

Summary

Background & Motivation

qingyang-hu Mar 20, 2024

Choose a reason for hiding this comment

mongodb-drivers-pr-bot bot commented Mar 20, 2024

API Change Report

./x/mongo/driver

incompatible changes

compatible changes

./x/mongo/driver/auth

incompatible changes

compatible changes

./x/mongo/driver/drivertest

incompatible changes

compatible changes

./x/mongo/driver/mnet

incompatible changes

./x/mongo/driver/operation

incompatible changes

./x/mongo/driver/session

incompatible changes

./x/mongo/driver/topology

incompatible changes

compatible changes

matthewdale left a comment • edited

Choose a reason for hiding this comment

matthewdale left a comment

Choose a reason for hiding this comment

matthewdale Apr 26, 2024

Choose a reason for hiding this comment

matthewdale Apr 26, 2024

Choose a reason for hiding this comment

matthewdale Apr 26, 2024

Choose a reason for hiding this comment

matthewdale Apr 26, 2024

Choose a reason for hiding this comment

matthewdale Apr 26, 2024

Choose a reason for hiding this comment

matthewdale Apr 26, 2024

Choose a reason for hiding this comment

prestonvasquez left a comment

Choose a reason for hiding this comment

matthewdale left a comment •

edited