Nicolas Hillegeer 87fded5d2a internal/impl: ensure proto.HasExtension does not allocate ... 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>		2024-03-28 16:31:28 +00:00
..
race	internal/impl: ensure proto.HasExtension does not allocate	2024-03-28 16:31:28 +00:00