mirror of
https://github.com/protocolbuffers/protobuf-go.git
synced 2024-12-29 12:17:48 +00:00
87fded5d2a
Extensions are unmarshaled lazily if protolegacy is true. The current
implementation of proto.HasExtension forces this unmarshaling to happen.
Change that.
Lazy message extensions are unmarshaled on first access, see
(*ExtensionField).Value. This leads to an (expensive) unmarshal
operation even if the user only wanted to know whether the extension is
present.
Granted, in most cases a HasExtension returning true will be followed by
a GetExtension. Due to memoization (see (*ExtensionField).lazyInit), the
cost will just shift from HasExtension to GetExtension. But, this CL
allows building cheaper functionality that only needs to know about
extension existence.
Why can this validation be removed?
- All tests pass.
- This check was added in CL 229558. The author (Joe Tsai) noted:
> Technically this shouldn't be needed, but I couldn't adequately reason
> whether a nil message value would ever be set through non-reflection
> means.
Like the author, I believe it's not needed:
- `proto.SetExtension` does not allow setting invalid messages (see
proto/extension.go).
- Likewise, (*extensionMap).Set panics when attempting to set an
invalid value.
- Unmarshaling does not produce submessages for which `IsValid` is
false.
The added test fails without the fix:
$ go test -tags=protolegacy -test.v -test.run=TestHasExtensionNoAlloc proto/extension_test.go
=== RUN TestHasExtensionNoAlloc
=== RUN TestHasExtensionNoAlloc/Nil
=== RUN TestHasExtensionNoAlloc/Eager
=== RUN TestHasExtensionNoAlloc/Lazy
extension_test.go:156: proto.HasExtension should not allocate, but allocated 3.00B per run
--- FAIL: TestHasExtensionNoAlloc (0.00s)
--- PASS: TestHasExtensionNoAlloc/Nil (0.00s)
--- PASS: TestHasExtensionNoAlloc/Eager (0.00s)
--- FAIL: TestHasExtensionNoAlloc/Lazy (0.00s)
FAIL
FAIL command-line-arguments 0.018s
The tests are disabled in race mode because the race instrumentation for
closures et al. always allocates. The protolegacy tests were previously
only run in race mode. I added a non-race variant in
integration_test.go.
Change-Id: Idbc67c1cf0aea8833a2735ca7bfc8d2466ceaf44
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/575035
Reviewed-by: Nicolas Hillegeer <aktau@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
Auto-Submit: Nicolas Hillegeer <aktau@google.com>
|
||
|---|---|---|
| .. | ||
| api_export.go | ||
| checkinit.go | ||
| codec_extension.go | ||
| codec_field.go | ||
| codec_gen.go | ||
| codec_map_go111.go | ||
| codec_map_go112.go | ||
| codec_map.go | ||
| codec_message.go | ||
| codec_messageset.go | ||
| codec_reflect.go | ||
| codec_tables.go | ||
| codec_unsafe.go | ||
| convert_list.go | ||
| convert_map.go | ||
| convert.go | ||
| decode.go | ||
| encode.go | ||
| enum_test.go | ||
| enum.go | ||
| extension_test.go | ||
| extension.go | ||
| lazy_test.go | ||
| legacy_aberrant_test.go | ||
| legacy_enum.go | ||
| legacy_export_test.go | ||
| legacy_export.go | ||
| legacy_extension.go | ||
| legacy_file_test.go | ||
| legacy_file.go | ||
| legacy_message.go | ||
| legacy_test.go | ||
| merge_gen.go | ||
| merge.go | ||
| message_reflect_field.go | ||
| message_reflect_gen.go | ||
| message_reflect_test.go | ||
| message_reflect.go | ||
| message.go | ||
| pointer_reflect.go | ||
| pointer_unsafe.go | ||
| validate.go | ||
| weak.go | ||