Skip to content

Commit a6c8e56

Browse files
osa1osa1
authored
Improve enum decoding -- alternative (#981)
When decoding an enum value, we call a callback in the enum field's `FieldInfo`. The callback then indexes a map mapping enum numbers to Dart values. When these conditions hold: - The known enum numbers are all positive. (so that we can use a list and index it with the number) - The known enum numbers are more than 70% of the large known enum number. (so that the list won't have a lot of `null` entries, wasting heap space) We now generate a list instead of a map to map enum numbers to Dart values. Similar to the map, the list is runtime allocated. No new code generated per message or enum type. AOT benchmarks: - Before: `protobuf_PackedEnumDecoding(RunTimeRaw): 47585.14 us.` - After: `protobuf_PackedEnumDecoding(RunTimeRaw): 38974.566666666666 us.` - Diff: -18% Wasm benchmarks: - Before: `protobuf_PackedEnumDecoding(RunTimeRaw): 52225.0 us.` - After: `protobuf_PackedEnumDecoding(RunTimeRaw): 34283.33333333333 us.` - Diff: -34% **Alternatives considered:** - #980 uses a map always, but eliminates the `valueOf` closure. - #985 uses a list always, and does binary search in the list when the list is "shallow". - #987 is the same as #985, but instead of calling the `valueOf` closure it stores an extra field in `FieldInfo`s for whether to binary search or directly index. These are all slower than the current PR.
1 parent 376c9ce commit a6c8e56

File tree

12 files changed

+171
-42
lines changed

12 files changed

+171
-42
lines changed

‎protobuf/lib/src/protobuf/protobuf_enum.dart‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
// for details. All rights reserved. Use of this source code is governed by a
33
// BSD-style license that can be found in the LICENSE file.
44

5+
// ignore_for_file: non_constant_identifier_names
6+
57
part of '../../protobuf.dart';
68

79
/// A base class for all proto enum types.
@@ -37,12 +39,28 @@ class ProtobufEnum {
3739
/// Creates a new constant [ProtobufEnum] using [value] and [name].
3840
const ProtobufEnum(this.value, this.name);
3941

40-
/// Creates a Map for all of the [ProtobufEnum]s in [byIndex], mapping each
42+
/// This function is for generated code.
43+
///
44+
/// Creates a Map for all of the [ProtobufEnum]s in [enumValues], mapping each
4145
/// [ProtobufEnum]'s [value] to the [ProtobufEnum].
42-
static Map<int, T> initByValue<T extends ProtobufEnum>(List<T> byIndex) {
46+
///
47+
/// @nodoc
48+
static Map<int, T> initByValue<T extends ProtobufEnum>(List<T> enumValues) {
4349
final byValue = <int, T>{};
44-
for (final v in byIndex) {
45-
byValue[v.value] = v;
50+
for (final enumValue in enumValues) {
51+
byValue[enumValue.value] = enumValue;
52+
}
53+
return byValue;
54+
}
55+
56+
/// This function is for generated code.
57+
///
58+
/// @nodoc
59+
static List<T?> $_initByValueList<T extends ProtobufEnum>(
60+
List<T> enumValues, int maxEnumValue) {
61+
final byValue = List<T?>.filled(maxEnumValue + 1, null);
62+
for (final enumValue in enumValues) {
63+
byValue[enumValue.value] = enumValue;
4664
}
4765
return byValue;
4866
}

‎protoc_plugin/Makefile‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ TEST_PROTO_LIST = \
3030
entity \
3131
enum_extension \
3232
enum_name \
33+
enums \
3334
extend_unittest \
3435
ExtensionEnumNameConflict \
3536
ExtensionNameConflict \

‎protoc_plugin/lib/src/enum_generator.dart‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -175,11 +175,37 @@ class EnumGenerator extends ProtobufContainer {
175175
out.println('];');
176176
out.println();
177177

178-
out.println(
179-
'static final $coreImportPrefix.Map<$coreImportPrefix.int, $classname> _byValue ='
180-
' $protobufImportPrefix.ProtobufEnum.initByValue(values);');
181-
out.println('static $classname? valueOf($coreImportPrefix.int value) =>'
182-
' _byValue[value];');
178+
var maxEnumValue = -1;
179+
for (final valueDescriptor in _canonicalValues) {
180+
if (valueDescriptor.number.isNegative) {
181+
maxEnumValue = -1; // don't use list
182+
break;
183+
}
184+
if (valueDescriptor.number > maxEnumValue) {
185+
maxEnumValue = valueDescriptor.number;
186+
}
187+
}
188+
189+
final useList = _canonicalValues.isEmpty ||
190+
(maxEnumValue >= 0 &&
191+
_canonicalValues.length / (maxEnumValue + 1) >= 0.7);
192+
193+
if (useList) {
194+
out.println(
195+
'static final $coreImportPrefix.List<$classname?> _byValue ='
196+
' $protobufImportPrefix.ProtobufEnum.\$_initByValueList(values, $maxEnumValue);');
197+
198+
out.println('static $classname? valueOf($coreImportPrefix.int value) =>'
199+
' value < 0 || value >= _byValue.length ? null : _byValue[value];');
200+
} else {
201+
out.println(
202+
'static final $coreImportPrefix.Map<$coreImportPrefix.int, $classname> _byValue ='
203+
' $protobufImportPrefix.ProtobufEnum.initByValue(values);');
204+
205+
out.println('static $classname? valueOf($coreImportPrefix.int value) =>'
206+
' _byValue[value];');
207+
}
208+
183209
out.println();
184210

185211
out.println('const $classname._(super.v, super.n);');

‎protoc_plugin/lib/src/generated/descriptor.pbenum.dart‎

Lines changed: 22 additions & 18 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎protoc_plugin/test/generated_message_test.dart‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import '../out/protos/constructor_args/google/protobuf/unittest.pb.dart'
1010
import '../out/protos/constructor_args/google/protobuf/unittest_import.pb.dart'
1111
as constructor_args_unittest_import;
1212
import '../out/protos/duplicate_names_import.pb.dart';
13+
import '../out/protos/enums.pb.dart';
1314
import '../out/protos/google/protobuf/unittest.pb.dart';
1415
import '../out/protos/google/protobuf/unittest_import.pb.dart';
1516
import '../out/protos/google/protobuf/unittest_optimize_for.pb.dart';
@@ -890,4 +891,27 @@ void main() {
890891
// constructor arguments, to be able to reuse `assertAllFieldsSet`.
891892
assertAllFieldsSet(TestAllTypes.fromBuffer(value.writeToBuffer()));
892893
});
894+
895+
test('Handling enums defined out of order', () {
896+
final message = MessageWithEnums();
897+
for (final enum_ in DenseEnum.values) {
898+
message.denseEnums.add(enum_);
899+
}
900+
for (final enum_ in DenseEnumOutOfOrder.values) {
901+
message.denseOutOfOrderEnums.add(enum_);
902+
}
903+
for (final enum_ in SparseEnum.values) {
904+
message.sparseEnums.add(enum_);
905+
}
906+
for (final enum_ in SparseEnumOutOfOrder.values) {
907+
message.sparseOutOfOrderEnums.add(enum_);
908+
}
909+
910+
final encoded = message.writeToBuffer();
911+
final decoded = MessageWithEnums.fromBuffer(encoded);
912+
expect(decoded.denseEnums, DenseEnum.values);
913+
expect(decoded.denseOutOfOrderEnums, DenseEnumOutOfOrder.values);
914+
expect(decoded.sparseEnums, SparseEnum.values);
915+
expect(decoded.sparseOutOfOrderEnums, SparseEnumOutOfOrder.values);
916+
});
893917
}

‎protoc_plugin/test/goldens/deprecations.pbenum‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,10 @@ class A extends $pb.ProtobufEnum {
2424
A2,
2525
];
2626

27-
static final $core.Map<$core.int, A> _byValue =
28-
$pb.ProtobufEnum.initByValue(values);
29-
static A? valueOf($core.int value) => _byValue[value];
27+
static final $core.List<A?> _byValue =
28+
$pb.ProtobufEnum.$_initByValueList(values, 1);
29+
static A? valueOf($core.int value) =>
30+
value < 0 || value >= _byValue.length ? null : _byValue[value];
3031

3132
const A._(super.v, super.n);
3233
}

‎protoc_plugin/test/goldens/doc_comments.pbenum‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,10 @@ class A extends $pb.ProtobufEnum {
2626
A2,
2727
];
2828

29-
static final $core.Map<$core.int, A> _byValue =
30-
$pb.ProtobufEnum.initByValue(values);
31-
static A? valueOf($core.int value) => _byValue[value];
29+
static final $core.List<A?> _byValue =
30+
$pb.ProtobufEnum.$_initByValueList(values, 1);
31+
static A? valueOf($core.int value) =>
32+
value < 0 || value >= _byValue.length ? null : _byValue[value];
3233

3334
const A._(super.v, super.n);
3435
}

‎protoc_plugin/test/goldens/enum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ class PhoneType extends $pb.ProtobufEnum {
1111
WORK,
1212
];
1313

14-
static final $core.Map<$core.int, PhoneType> _byValue = $pb.ProtobufEnum.initByValue(values);
15-
static PhoneType? valueOf($core.int value) => _byValue[value];
14+
static final $core.List<PhoneType?> _byValue = $pb.ProtobufEnum.$_initByValueList(values, 2);
15+
static PhoneType? valueOf($core.int value) => value < 0 || value >= _byValue.length ? null : _byValue[value];
1616

1717
const PhoneType._(super.v, super.n);
1818
}

‎protoc_plugin/test/goldens/messageGeneratorEnums‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ class PhoneNumber_PhoneType extends $pb.ProtobufEnum {
1111
WORK,
1212
];
1313

14-
static final $core.Map<$core.int, PhoneNumber_PhoneType> _byValue = $pb.ProtobufEnum.initByValue(values);
15-
static PhoneNumber_PhoneType? valueOf($core.int value) => _byValue[value];
14+
static final $core.List<PhoneNumber_PhoneType?> _byValue = $pb.ProtobufEnum.$_initByValueList(values, 2);
15+
static PhoneNumber_PhoneType? valueOf($core.int value) => value < 0 || value >= _byValue.length ? null : _byValue[value];
1616

1717
const PhoneNumber_PhoneType._(super.v, super.n);
1818
}

0 commit comments

Comments
 (0)