Skip to content

Improve enum decoding #980

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Improve enum decoding #980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78c88d9
Remove closure calls when mapping enum nums to vals
osa1 May 7, 2025
6fe139a
Rename valueOfMap -> enumValueMap
osa1 May 7, 2025
0f51158
Remove valueOf
osa1 May 7, 2025
bd1ca9f
Fix warn
osa1 May 7, 2025
b887c49
Merge remote-tracking branch 'origin/master' into improve_enum_decoding
osa1 May 9, 2025
b81767f
Merge remote-tracking branch 'origin/master' into improve_enum_decoding
osa1 May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 17 additions & 23 deletions protobuf/lib/src/protobuf/builder_info.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class BuilderInfo {
int? fieldType,
dynamic defaultOrMaker,
CreateBuilderFunc? subBuilder,
ValueOfFunc? valueOf,
Map<int, ProtobufEnum>? enumValueMap,
List<ProtobufEnum>? enumValues,
{String? protoName}) {
final index = byIndex.length;
Expand All @@ -70,7 +70,7 @@ class BuilderInfo {
: FieldInfo<T>(name, tagNumber, index, fieldType!,
defaultOrMaker: defaultOrMaker,
subBuilder: subBuilder,
valueOf: valueOf,
enumValueMap: enumValueMap,
enumValues: enumValues,
protoName: protoName);
_addField(fieldInfo);
Expand All @@ -97,14 +97,14 @@ class BuilderInfo {
int fieldType,
CheckFunc<T> check,
CreateBuilderFunc? subBuilder,
ValueOfFunc? valueOf,
Map<int, ProtobufEnum>? enumValueMap,
List<ProtobufEnum>? enumValues,
{ProtobufEnum? defaultEnumValue,
String? protoName}) {
final index = byIndex.length;
_addField(FieldInfo<T>.repeated(
name, tagNumber, index, fieldType, check, subBuilder,
valueOf: valueOf,
enumValueMap: enumValueMap,
enumValues: enumValues,
defaultEnumValue: defaultEnumValue,
protoName: protoName));
Expand All @@ -126,10 +126,10 @@ class BuilderInfo {
void a<T>(int tagNumber, String name, int fieldType,
{dynamic defaultOrMaker,
CreateBuilderFunc? subBuilder,
ValueOfFunc? valueOf,
Map<int, ProtobufEnum>? enumValueMap,
List<ProtobufEnum>? enumValues,
String? protoName}) {
add<T>(tagNumber, name, fieldType, defaultOrMaker, subBuilder, valueOf,
add<T>(tagNumber, name, fieldType, defaultOrMaker, subBuilder, enumValueMap,
enumValues,
protoName: protoName);
}
Expand Down Expand Up @@ -169,11 +169,11 @@ class BuilderInfo {
// Enum.
void e<T>(int tagNumber, String name, int fieldType,
{dynamic defaultOrMaker,
ValueOfFunc? valueOf,
Copy link
Collaborator

@mkustermann mkustermann May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these changes removing those parameters from public APIs? They seem to be breaking changes. So old generated code will no longer work with newest package:protobuf. If package:protobuf has to upgrade to new major version then it causes trouble for ecosystem:

If an app depends transitively on A and B and A uses old and B uses new protobuf version, then we get constraint conflict.

Copy link
Member Author

@osa1 osa1 May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a breaking change. You need to update protoc_plugin and protobuf together and re-generate your proto classes.

We've done it in the past, for example with the most recent protobuf and protoc_plugins.

If an app depends transitively on A and B and A uses old and B uses new protobuf version, then we get constraint conflict.

Yes, but you can't do any major bumps and breaking changes if you want to avoid this. We deal with it the same way we've dealt with it in the past.

Copy link
Collaborator

@mkustermann mkustermann May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from your experience it doesn't cause too much churn on the ecosystem? Then it's ok, but you'll have to bump major version number.

Copy link
Member Author

@osa1 osa1 May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from your experience it doesn't cause too much churn on the ecosystem?

I don't have too much experience update downstream with major version bumps, but I expect this particular change to not be a big problem.

Maybe someone who works on updating dependencies can comment how much churn this kind of thing causes.

Internally, no one notice this change other than maybe their benchmarks getting faster.

Copy link
Contributor

@devoncarew devoncarew May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Releasing a major version (w/ planning, batching breaking changes, planning deprecations, ...) should be just a normal part of the process. The diamond constraint issue becomes a problem when one of the packages in the cycle has become unmaintained (rev'ing their deps doesn't happen or happens w/ lots of latency). You're more likely to run into that when more things depend on the package being released - when it's further down the overall ecosystem stack.

For something like protobuf I'd just do regular planning for a new major version but would still rev. when necessary. Even for breaking changes, you want to minimize the work for customers - they will eventually need to rev from 3 => 4, or 4 => 5.

There are a few packages that are very core to the ecosystem, and you have to be cautious about rev'ing, or choose to not rev at all (like, the direct deps of flutter - https://github.com/flutter/flutter/blob/master/packages/flutter/pubspec.yaml#L8).

Map<int, ProtobufEnum>? enumValueMap,
List<ProtobufEnum>? enumValues,
String? protoName}) {
add<T>(
tagNumber, name, fieldType, defaultOrMaker, null, valueOf, enumValues,
add<T>(tagNumber, name, fieldType, defaultOrMaker, null, enumValueMap,
enumValues,
protoName: protoName);
}

Expand All @@ -188,13 +188,13 @@ class BuilderInfo {
// Repeated message, group, or enum.
void pc<T>(int tagNumber, String name, int fieldType,
{CreateBuilderFunc? subBuilder,
ValueOfFunc? valueOf,
Map<int, ProtobufEnum>? enumValueMap,
List<ProtobufEnum>? enumValues,
ProtobufEnum? defaultEnumValue,
String? protoName}) {
assert(_isGroupOrMessage(fieldType) || _isEnum(fieldType));
addRepeated<T>(tagNumber, name, fieldType, _checkNotNull, subBuilder,
valueOf, enumValues,
enumValueMap, enumValues,
defaultEnumValue: defaultEnumValue, protoName: protoName);
}

Expand Down Expand Up @@ -237,7 +237,7 @@ class BuilderInfo {
required int keyFieldType,
required int valueFieldType,
CreateBuilderFunc? valueCreator,
ValueOfFunc? valueOf,
Map<int, ProtobufEnum>? enumValueMap,
List<ProtobufEnum>? enumValues,
ProtobufEnum? defaultEnumValue,
PackageName packageName = const PackageName(''),
Expand All @@ -247,7 +247,7 @@ class BuilderInfo {
package: packageName)
..add(PbMap._keyFieldNumber, 'key', keyFieldType, null, null, null, null)
..add(PbMap._valueFieldNumber, 'value', valueFieldType,
valueDefaultOrMaker, valueCreator, valueOf, enumValues);
valueDefaultOrMaker, valueCreator, enumValueMap, enumValues);

addMapField<K, V>(tagNumber, name, keyFieldType, valueFieldType,
mapEntryBuilderInfo, valueCreator,
Expand Down Expand Up @@ -287,11 +287,6 @@ class BuilderInfo {
return i?.tagNumber;
}

ValueOfFunc? valueOfFunc(int tagNumber) {
final i = fieldInfo[tagNumber];
return i?.valueOf;
}

/// The [FieldInfo] for each field in tag number order.
List<FieldInfo> get sortedByTag => _sortedByTag ??= _computeSortedByTag();

Expand Down Expand Up @@ -323,13 +318,12 @@ class BuilderInfo {

ProtobufEnum? _decodeEnum(
int tagNumber, ExtensionRegistry? registry, int rawValue) {
final f = valueOfFunc(tagNumber);
if (f != null) {
return f(rawValue);
final valueMap = fieldInfo[tagNumber]?.enumValueMap;
if (valueMap != null) {
return valueMap[rawValue];
}
return registry
?.getExtension(qualifiedMessageName, tagNumber)
?.valueOf
?.call(rawValue);
?.enumValueMap?[rawValue];
}
}
10 changes: 6 additions & 4 deletions protobuf/lib/src/protobuf/extension.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,26 @@ class Extension<T> extends FieldInfo<T> {
Extension(this.extendee, String name, int tagNumber, int fieldType,
{dynamic defaultOrMaker,
CreateBuilderFunc? subBuilder,
ValueOfFunc? valueOf,
Map<int, ProtobufEnum>? enumValueMap,
List<ProtobufEnum>? enumValues,
String? protoName})
: super(name, tagNumber, null, fieldType,
defaultOrMaker: defaultOrMaker,
subBuilder: subBuilder,
valueOf: valueOf,
enumValueMap: enumValueMap,
enumValues: enumValues,
protoName: protoName);

Extension.repeated(this.extendee, String name, int tagNumber, int fieldType,
{required CheckFunc<T> check,
CreateBuilderFunc? subBuilder,
ValueOfFunc? valueOf,
Map<int, ProtobufEnum>? enumValueMap,
List<ProtobufEnum>? enumValues,
String? protoName})
: super.repeated(name, tagNumber, null, fieldType, check, subBuilder,
valueOf: valueOf, enumValues: enumValues, protoName: protoName);
enumValueMap: enumValueMap,
enumValues: enumValues,
protoName: protoName);

@override
int get hashCode => extendee.hashCode * 31 + tagNumber;
Expand Down
17 changes: 10 additions & 7 deletions protobuf/lib/src/protobuf/field_info.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class FieldInfo<T> {
/// Mapping from enum integer values to enum values.
///
/// Only available in enum fields.
final ValueOfFunc? valueOf;
final Map<int, ProtobufEnum>? enumValueMap;

/// Function to verify items when adding to a repeated field.
///
Expand All @@ -101,7 +101,7 @@ class FieldInfo<T> {
FieldInfo(this.name, this.tagNumber, this.index, this.type,
{dynamic defaultOrMaker,
this.subBuilder,
this.valueOf,
this.enumValueMap,
Copy link
Collaborator

@mkustermann mkustermann May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of getting this new parameter, why can't we take enumValues we already get and calculate this based on that? And if we can do that, we can decide whether we calculate a list or a map depending on density of the enum values, etc.

Copy link
Member Author

@osa1 osa1 May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of getting this new parameter, why can't we take enumValues we already get and calculate this based on that? And if we can do that, we can decide whether we calculate a list or a map depending on density of the enum values, etc.

We have one function to decode enum int values:

protobuf.dart/protobuf/lib/src/protobuf/builder_info.dart

Lines 324 to 334 in 006d3aa

ProtobufEnum? _decodeEnum(
int tagNumber, ExtensionRegistry? registry, int rawValue) {
final f = valueOfFunc(tagNumber);
if (f != null) {
return f(rawValue);
}
return registry
?.getExtension(qualifiedMessageName, tagNumber)
?.valueOf
?.call(rawValue);
}

For it to index a list sometimes and map other times, we would need a type test, or a closure, as in master branch and the alternative PR.

Re: generating the list or map from the enumValues argument: the map and list should be per-type, but runtime allocated. So it needs to be in a final static in the generated the enum classes. Example:

protobuf.dart/protoc_plugin/lib/src/generated/plugin.pbenum.dart

Lines 30 to 31 in 006d3aa

static final $core.Map<$core.int, CodeGeneratorResponse_Feature> _byValue =
$pb.ProtobufEnum.initByValue(values);

And then this static field needs to be passed to FieldInfo so that BuilderInfo can find it from the FieldInfo.

Does this make sense?

Copy link
Collaborator

@mkustermann mkustermann May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

But instead of passing the representation (e.g. Map<int, XX>, List<XX> in here) or a valueOf closure. One could pass an instance of a ProtobufEnumDescriptor in here that has a .decode() method.

This could be a direct call, it could have a List<T?> values and a bool (whether it's indexed by value or binary search). It would then lookup and return. So there wouldn't be indirect calls (neither closure calls nor dispatch table calls). We would also pass the integer argument unboxed (in closure call we pass it boxed).

Something like

// package:protobuf
class ProtobufEnum {}
class ProtobufEnumDescriptor {
  final bool _binarySearch;
  final List<ProtobufEnum?> _byValue;
  ProtobufEnumDescriptor(this._binarySearch, this._byValue);  

  ProtobufEnum? lookup(int value) { ... }
}

// foo.pbenum.dart
class FooEnum extends ProtobufEnum {
  ...
  static final info_ = ProtobufEnumDescriptor(values, useBinarySearch: <if sparse>);
}

// foo.pb.dart

class Foo extends GeneratedMessage {
  final info_ = BuilderInfo()
    ..enumField(FooEnum.info_);
}

Then the decoder would see

  ProtobufEnum? _decodeEnum(int tagNumber, ExtensionRegistry? registry, int rawValue) {
    final f = enumDescriptorOf(tagNumber);
    if (f != null) {
      return f.lookup(rawValue); // direct call, passing `rawValue` unboxed, does array lookup or binary search
    }
    ...;
  }

Would that work?
If so, wouldn't that be the most efficient way to do it?

Copy link
Member Author

@osa1 osa1 May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would work and I agree that it should improve things.

Compared to the current PR, it's similar to passing a list (instead of a map) + a bool to whether the list should be binary searched or indexed.

A BuilderInfo.enumField wouldn't work. The enum infos need to be attached to field infos for repeated and map fields as well, so we need to pass the info to BuilderInfo.m (adds map field info) and BuilderInfo.addRepeated.

An class ProtobufEnumDescriptor would also mean a level of indirection when accessing the list and the boolean flag for whether to binary search. So perhaps passing an extra argument to the current methods for the bool flag would perform the best.

I'll first update the other PR with binary search because that's easy to do. Then update benchmarks to have some sparse enums/enums with large encodings. We can merge the benchmarks separately. Then revisit this idea.

this.enumValues,
this.defaultEnumValue,
String? protoName})
Expand All @@ -112,7 +112,7 @@ class FieldInfo<T> {
assert(!_isGroupOrMessage(type) ||
subBuilder != null ||
_isMapField(type)),
assert(!_isEnum(type) || valueOf != null);
assert(!_isEnum(type) || enumValueMap != null);

// Represents a field that has been removed by a program transformation.
FieldInfo.dummy(this.index)
Expand All @@ -121,19 +121,22 @@ class FieldInfo<T> {
tagNumber = 0,
type = 0,
makeDefault = null,
valueOf = null,
enumValueMap = null,
check = null,
enumValues = null,
defaultEnumValue = null,
subBuilder = null;

FieldInfo.repeated(this.name, this.tagNumber, this.index, this.type,
CheckFunc<T> this.check, this.subBuilder,
{this.valueOf, this.enumValues, this.defaultEnumValue, String? protoName})
{this.enumValueMap,
this.enumValues,
this.defaultEnumValue,
String? protoName})
: makeDefault = (() => PbList<T>(check: check)),
_protoName = protoName,
assert(_isRepeated(type)),
assert(!_isEnum(type) || valueOf != null);
assert(!_isEnum(type) || enumValueMap != null);

static MakeDefaultFunc? findMakeDefault(int type, dynamic defaultOrMaker) {
if (defaultOrMaker == null) return PbFieldType._defaultForType(type);
Expand Down Expand Up @@ -277,7 +280,7 @@ class MapFieldInfo<K, V> extends FieldInfo<PbMap<K, V>?> {
defaultOrMaker: () => PbMap<K, V>(keyFieldType, valueFieldType),
defaultEnumValue: defaultEnumValue,
protoName: protoName) {
assert(!_isEnum(type) || valueOf != null);
assert(!_isEnum(type) || enumValueMap != null);
}

FieldInfo get valueFieldInfo =>
Expand Down
2 changes: 1 addition & 1 deletion protobuf/lib/src/protobuf/proto3_json.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ void _mergeFromProto3Json(
if ((result != null) || ignoreUnknownFields) return result;
throw context.parseException('Unknown enum value', value);
} else if (value is int) {
return fieldInfo.valueOf!(value) ??
return fieldInfo.enumValueMap![value] ??
(ignoreUnknownFields
? null
: (throw context.parseException(
Expand Down
4 changes: 2 additions & 2 deletions protobuf/test/mock_util.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'package:collection/collection.dart';
import 'package:fixnum/fixnum.dart' show Int64;
import 'package:protobuf/protobuf.dart'
show
Expand All @@ -23,7 +22,8 @@ BuilderInfo mockInfo(String className, CreateBuilderFunc create) {
// 6 is reserved for extensions in other tests.
..e(7, 'enm', PbFieldType.OE,
defaultOrMaker: mockEnumValues.first,
valueOf: (i) => mockEnumValues.firstWhereOrNull((e) => e.value == i),
enumValueMap:
Map.fromEntries(mockEnumValues.map((e) => MapEntry(e.value, e))),
enumValues: mockEnumValues);
}

Expand Down
4 changes: 1 addition & 3 deletions protoc_plugin/lib/src/enum_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,8 @@ class EnumGenerator extends ProtobufContainer {
out.println();

out.println(
'static final $coreImportPrefix.Map<$coreImportPrefix.int, $classname> _byValue ='
'static final $coreImportPrefix.Map<$coreImportPrefix.int, $classname> byValue ='
' $protobufImportPrefix.ProtobufEnum.initByValue(values);');
out.println('static $classname? valueOf($coreImportPrefix.int value) =>'
' _byValue[value];');
out.println();

out.println('const $classname._(super.v, super.n);');
Expand Down
4 changes: 2 additions & 2 deletions protoc_plugin/lib/src/extension_generator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class ExtensionGenerator {
if (type.isMessage || type.isGroup) {
named['subBuilder'] = '$dartType.create';
} else if (type.isEnum) {
named['valueOf'] = '$dartType.valueOf';
named['enumValueMap'] = '$dartType.byValue';
named['enumValues'] = '$dartType.values';
}
} else {
Expand All @@ -129,7 +129,7 @@ class ExtensionGenerator {
named['subBuilder'] = '$dartType.create';
} else if (type.isEnum) {
final dartEnum = type.getDartType(fileGen!);
named['valueOf'] = '$dartEnum.valueOf';
named['enumValueMap'] = '$dartEnum.byValue';
named['enumValues'] = '$dartEnum.values';
}
}
Expand Down
12 changes: 6 additions & 6 deletions protoc_plugin/lib/src/generated/descriptor.pb.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 6 additions & 14 deletions protoc_plugin/lib/src/generated/descriptor.pbenum.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading