Skip to content

Commit

Permalink
protoreflect: remove unnecessary cache so extension types can be GC'ed
Browse files Browse the repository at this point in the history
Resolves golang/protobuf#1521

In the internal/impl package, a helper computes metadata used for
serialization and deserialization of extension values. It features a
package var of type sync.Map that is used as cache. This ostensibly
was for performance, however it has never worked, because the code
that updates the cache inserts entries into the wrong map. (These
erroneous entries do not cause any issues because they are keys that
never conflict with those used in valid queries.)

Instead of a one-line fix to have this code update the correct cache,
this change removes the cache altogether. The existence of the cache
means that once a protoreflect.ExtensionType is used to serialize or
deserialize a message, it can *never* be garbage collected. Workloads
that are long-lived servers using dynamic messages and extensions
based on user requests will exhibit unbounded growth in memory usage
as the cache only gets larger and larger.

Since the cache has never worked, any advantages it ostensibly
conferred have not been missed. So this fixes the unbounded memory
growth instead.

Change-Id: I15957fd8521852f9f7f9f89db7ebfd7170d85202
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/560095
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
  • Loading branch information
jhump authored and gopherbot committed Feb 1, 2024
1 parent b70f02b commit 82c6b3a
Showing 1 changed file with 7 additions and 15 deletions.
22 changes: 7 additions & 15 deletions internal/impl/codec_extension.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,26 +21,18 @@ type extensionFieldInfo struct {
validation validationInfo
}

var legacyExtensionFieldInfoCache sync.Map // map[protoreflect.ExtensionType]*extensionFieldInfo

func getExtensionFieldInfo(xt protoreflect.ExtensionType) *extensionFieldInfo {
if xi, ok := xt.(*ExtensionInfo); ok {
xi.lazyInit()
return xi.info
}
return legacyLoadExtensionFieldInfo(xt)
}

// legacyLoadExtensionFieldInfo dynamically loads a *ExtensionInfo for xt.
func legacyLoadExtensionFieldInfo(xt protoreflect.ExtensionType) *extensionFieldInfo {
if xi, ok := legacyExtensionFieldInfoCache.Load(xt); ok {
return xi.(*extensionFieldInfo)
}
e := makeExtensionFieldInfo(xt.TypeDescriptor())
if e, ok := legacyMessageTypeCache.LoadOrStore(xt, e); ok {
return e.(*extensionFieldInfo)
}
return e
// Ideally we'd cache the resulting *extensionFieldInfo so we don't have to
// recompute this metadata repeatedly. But without support for something like
// weak references, such a cache would pin temporary values (like dynamic
// extension types, constructed for the duration of a user request) to the
// heap forever, causing memory usage of the cache to grow unbounded.
// See discussion in https://github.com/golang/protobuf/issues/1521.
return makeExtensionFieldInfo(xt.TypeDescriptor())
}

func makeExtensionFieldInfo(xd protoreflect.ExtensionDescriptor) *extensionFieldInfo {
Expand Down

0 comments on commit 82c6b3a

Please sign in to comment.