-
Notifications
You must be signed in to change notification settings - Fork 213
Add a parse
static member to each enumerated type
#2348
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.
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
Comments
If anything, It should be a static method, not a getter, so A problem with adding any members to enums automatically, is that it blocks that name from being used as an enum element name. That's why I'm very hesitant about adding anything to all It's not at all clear that all enums need or want to have a We could introduce the We could allow you to declare an "abstract static" member, like |
Indeed. It's a little bit like However, a standard static |
I agree on both counts. It's weird when a namespace contains unrelated kinds of entities, and the namespace of an enum type exists primarily to expose the enum values. Could we do: Enum<SomeEnumType>.parse('valueName'); Which is defined something like: class Enum {
static T parse<T extends Enum>(String name) => ...
} Probably also: class Enum {
static T? tryParse<T extends Enum>(String name) => ...
} Of course, the implementation would have to be magic since there's no way to call the static |
The String name is also not really a good way of serializing an Enum value, usually I would use the index. It would feel pretty weird to me for us to directly support parsing, but use an inefficient mechanism to do so. Yes it is a bit "safer" but only in the circumstances where each side of the communication is compiled with a different version of the library, so you already are basically asking for trouble in that scenario. |
I mean, the string representation of a floating point number isn't a good way of serializing it, but we still let you parse those. :) I agree it's probably not a frequently used operation, but it does come up in real code and would be nice to have built-in support for it instead of having to re-implement it manually for every enum type where you need it. |
One could also choose to live with writing We don't have a nice |
A question - will parsing using Context: we parse enum values in dwds on every object returned from chrome several times, so I wondered what is the fastest way to do so. Update:
enum E {
object,
set,
list,
map,
function,
record,
type,
recordType,
nativeError,
nativeObject;
static E parse1(String value) {
return switch (value) {
'object' => object,
'set' => set,
'list' => list,
'map' => map,
'function' => function,
'record' => record,
'type' => type,
'recordType' => recordType,
'nativeError' => nativeError,
'nativeObject' => nativeObject,
_ => object,
};
}
static final parse2 = values.byName;
static final _valueMap = {
for (var v in values) '${v.name}': v,
};
static E parse3(String value) => _valueMap[value]!;
}
void main(List<String> args) {
const N = 1000000;
if (args[0] == 'one') {
{
final stopwatch = Stopwatch()..start();
for (var i = 0; i < N; i++) {
for (var v in E.values) {
var parsed = E.parse1(v.name);
}
}
print('Parse1: ${stopwatch.elapsedMilliseconds}ms');
}
} else if (args[0] == 'two') {
final stopwatch = Stopwatch()..start();
for (var i = 0; i < N; i++) {
for (var v in E.values) {
var parsed = E.parse2(v.name);
}
}
print('Parse2: ${stopwatch.elapsedMilliseconds}ms');
} else {
final stopwatch = Stopwatch()..start();
for (var i = 0; i < N; i++) {
for (var v in E.values) {
var parsed = E.parse3(v.name);
}
}
print('Parse3: ${stopwatch.elapsedMilliseconds}ms');
}
} Output ➜ dart try.dart one |
Specialized code for the particular enum will almost certainly be faster than generic code that runs through a list and checks the name of each element. The switch can be optimized in many ways by the compiler, up to an including generating a perfect hash, because it knows all the string values at compile-time. Unless it inlines the If your need for speed is great, writing the switch or map manually is a way to get it. We could (semi-easily) make extension EnumByName<E extends Enum> on List<E> {
static Expando<Map<String, Enum>> _nameMap = Expando();
E byName(String name) =>
((_nameMap[this] ??= {for (var v in this) v.name: v})[name] ??
(throw ArgumentError.value(
name, "name", "No enum value with that name"))) as E;
}
Biggest issue is that it's not correct for mutable lists. The extension applies to any list of enum values, |
Can you (1) characterize how important this is (e.g. it is 1% or 50% of your parsing) and (2) characterize the properties of the enums (e.g. number of enum members, is it expected to stay ~10 or grow to ~100 or more)
I don't believe that switch-on-string is optimized really well on any platform. It degenerates to a sequence of Since 5x is bigger than the effect you mention, which compiler you are using might be the most important factor for answering your question. The 'sometimes' part is difficult to characterize, but benchmarks often do things just right to get the 5x when in real life the benefit does not materialize. Benchmarking is hard and benchmarking against an adversary like a JIT is harder. |
(1) I don't have the data for my code but I presume that the percentage would be small (more like 1%). But that is just for my case, I wonder if other cases would prefer faster enums. (2) The number of enums will grow in my case with any special cases of an object created by DDC-compiled code that we need to translate to vm service Instances, but I would be surprised if it grows to more than ~20. To clarify - I was't looking into improving my specific case (I can use a map or a switch if we determine that enum parsing speed matters) but start a general discussion on what implementation is better (if this is important, we could create benchmarks).
Yeah I was thinking about const strings and stuff... so I think you are right. In the defense of the argument I could say that string comparison would not be worth going over array elements and performing string comparison:) But it looks like it gives an opportunity for other opts to kick in (like whatever worked in my case). I was using the VM on macos in my experiments. |
Thanks for the example and the explanation! It seems that we are currently optimizing for size - is it always our default choice for new features? I tried your example below (case 4) and also modified it to not extend a list (case 5), which should solve the problem for modifiable lists. They didn't perform as good as a static map on enum itself but are still a bit better than the current Not sure if this adds much value, please feel free to postpone:) enum E {
object,
set,
list,
map,
function,
record,
type,
recordType,
nativeError,
nativeObject;
static E parse1(String value) {
return switch (value) {
'object' => object,
'set' => set,
'list' => list,
'map' => map,
'function' => function,
'record' => record,
'type' => type,
'recordType' => recordType,
'nativeError' => nativeError,
'nativeObject' => nativeObject,
_ => object,
};
}
static final parse2 = values.byName;
static final _valueMap = {
for (var v in values) '${v.name}': v,
};
static E parse3(String value) => _valueMap[value]!;
static E parse4(String value) => E.values.byName2(value);
static E parse5(String value) => EnumByName2.byName3(value, E.values);
static E parse6(int i) => E.values[i];
}
extension EnumByName<N extends Enum> on List<N> {
static Expando<Map<String, dynamic>> _nameMap = new Expando();
N byName2(String name) =>
(_nameMap[this] ??= {for (var v in this) v.name: v})[name] ??
(throw ArgumentError.value(name, "name", "No enum value with that name"));
}
class EnumByName2 {
static Expando<Map<String, dynamic>> _nameMap = new Expando();
static N byName3<N extends Enum>(String name, List<N> values) =>
(_nameMap[N] ??= {for (var v in values) v.name: v})[name] ??
(throw ArgumentError.value(name, "name", "No enum value with that name"));
}
void main(List<String> args) {
const N = 1000000;
if (args[0] == 'one') {
{
final stopwatch = Stopwatch()..start();
for (var i = 0; i < N; i++) {
for (var v in E.values) {
var parsed = E.parse1(v.name);
}
}
print('Parse1: ${stopwatch.elapsedMilliseconds}ms');
}
} else if (args[0] == 'two') {
final stopwatch = Stopwatch()..start();
for (var i = 0; i < N; i++) {
for (var v in E.values) {
var parsed = E.parse2(v.name);
}
}
print('Parse2: ${stopwatch.elapsedMilliseconds}ms');
} else if (args[0] == 'three') {
final stopwatch = Stopwatch()..start();
for (var i = 0; i < N; i++) {
for (var v in E.values) {
var parsed = E.parse3(v.name);
}
}
print('Parse3: ${stopwatch.elapsedMilliseconds}ms');
} else if (args[0] == 'four') {
final stopwatch = Stopwatch()..start();
for (var i = 0; i < N; i++) {
for (var v in E.values) {
var parsed = E.parse4(v.name);
}
}
print('Parse4: ${stopwatch.elapsedMilliseconds}ms');
} else if (args[0] == 'five') {
final stopwatch = Stopwatch()..start();
for (var i = 0; i < N; i++) {
for (var v in E.values) {
var parsed = E.parse5(v.name);
}
}
print('Parse5: ${stopwatch.elapsedMilliseconds}ms');
} else if (args[0] == 'six') {
final stopwatch = Stopwatch()..start();
for (var i = 0; i < N; i++) {
for (var i = 0; i < E.values.length; i++) {
var parsed = E.parse6(i);
}
}
print('Parse6: ${stopwatch.elapsedMilliseconds}ms');
}
}
Results:
|
@annagrin have you tried just parsing by index? Unless you really care about human readable traffic over the network it should be much better. |
very true:) I was hoping that human readable form could be fast as well but it does not look like we have a clear winner now... |
Whether we optimize for speed or size depends on a few things (which mostly means gut-feeling and feedback from the web compiler teams). We always try to optimize for speed, within the constraints that we have, and the constraint is usually size related, like any of deployment code size, runtime memory usage or allocation churn. All of these are nice to reduce. Size is a bigger issue if the code is hard to tree-shake. If not using the feature gives you zero code-size in the generated code, it's better than if it's a constant overhead whether you use the feature or not. (In this case, a statc parse function would be a static function, which is perfectly tree-shakable.) A Even if code can be tree-shaken, a big piece of code that is almost always included, or smaller code that is included repeatedly like a parse function on each
I added the The We could build this map eagerly at compile-time, as a static const map per enum, and it should still be tree-shakable if not used, but it is not something we can use in general code which abstracts over enums (or if we do, we lose tree-shaking). Or we could add a static parse function per enum, at least those which does not declare another member named static MyEnum? tryParse(String source) {
if (source.startsWith('MyEnum.')) source = source.substring('MyEnum.'.length);
return switch (source) {
"foo" => MyEnum.foo,
"theBar" => MyEnum.theBar,
"quxxius" => MyEnum.quxxius,
_ => null;
};
} Not a lot of code, and a string switch, which is something we can optimize to be very efficient if we want to. Which brings us back to the original proposal, adding a My biggest gripe with a If the author can aske for the function, then it's different. Then I start worrying about having a single default implementation, which might not be what the enum author wants. (Do they want to allow You can override the I'm generally against automatic language-introduced (The The public The evolution path for auto-generated code when usage changes, is to add more configurability, or more different versions of the auto-generated code. Just updating to a new version of a macro package is much more flexible. So, my suggestion for enum parse methods is that the analyzer introduces a "quick-fix"/template which introduces a static |
That sounds useful! |
I would not add a method containing a switch as it is a maintenance burden. It can become inconsistent with the enum declaration. A switch expression or statement is bloated since it is essentially redundant with So we add a "quick-fix" I would suggest that it use enum MyEnum {
one,
two
} ⟷ enum MyEnum {
one,
two;
static MyEnum? tryParse(String source) {
// Comment out these lines to also accept strings with the enum name prefix:
// const prefix = 'MyEnum.';
// if (source.startsWith(prefix)) source = source.substring(prefix.length);
return _nameMap[source];
}
static final _nameMap = values.asNameMap();
} |
Cf. dart-lang/sdk#33244. Thanks to @kevmoo for making this proposal!
We can enable a convenient
parse
operation on each enumerated type using the following idiom:This issue is a proposal to implicitly induce such a static
parse
getter to each enumerated type (or a static method with the same usage, if that turns out to have better performance).Several points of motivation for doing this are mentioned here. The main point is that this translation from a string to an enum value is a commonly occurring task, and doing it should be obvious and convenient.
It could be argued that
E.one.toString()
yieldsE.one
, and hence theparse
method should expect (or at least tolerate) the input'E.one'
rather than just'one'
. It would be easy to write a wrapper function that strips offE.
if provided, or adds it if it is missing, but in this particular setting it is important to establish a good default, because it is all about convenience and brevity.@lrhn, WDYT?
The text was updated successfully, but these errors were encountered: