-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Enum.ToString is slower than explicit implementation #57748
Comments
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
|
@EgorBo A good reason would be that for example logging frameworks often use ToString to output components of the message. |
This could simply be a source generator. |
@Tornhoof Yes thats true. Does dotnet/runtime has already built-in source code generators? |
Yeah, in 6.0 source generators were added for JSON serializers and for logging. |
Can't this be a Roslyn code analyzer and fixer under the "performance" category? If the analyzer sees Note: This could in theory be a behavioral breaking change if new values are introduced to the enum between compile time and runtime. If the enum is defined in the same assembly as the call site, this obviously isn't an issue. But I don't think this would happen in practice often enough to be worth worrying about. |
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsDescriptionSince Enum.ToString usually accesses Enum.GetEnumName, which accesses it in the runtime. runtime/src/libraries/System.Private.CoreLib/src/System/Enum.cs Lines 1136 to 1146 in 895c99c
runtime/src/libraries/System.Private.CoreLib/src/System/Enum.cs Lines 135 to 147 in 895c99c
runtime/src/libraries/System.Private.CoreLib/src/System/Enum.cs Lines 124 to 133 in 895c99c
However, since the name is already given at compile time I think this should be treated as a constant and assembled at compile time. Configuration
DataBenchmarks
ReferencesEnum.GetEnumName IL
Extension method IL
We see the IL is bigger, but the code is faster. AnalysisIt is because in runtime the value is resolved although it is already known at compile time. Perhaps it would also make sense to add a External references
|
Well in many cases it is unknown which Enum Variant you get by .ToString i.e. you would always have to write such a huge switch construct. |
I don't follow. Why would an analyzer need a switch statement? The suggestion above was basically pattern recognition, with a few extra checks to make sure the substitution is legal. |
Oh so that's what you mean, would have assumed you meant creating an analyzer in general, for .ToString calls to enums and not their variants. |
Ok, thanks for the clarification. Let me repeat what I believe your scenario and proposal are so that we're all clear here. In your scenario, you have an enum of type MyEnum whose value is not a compile-time or JIT-time constant, perhaps because the value is read from Is this correct? |
I believe @deeprobin refers to the following use case: void PrintEnum(SomeEnum value) // value not known during compile time
{
Console.WriteLine(value.ToString());
} |
@giladfrid009 Exactly, you got that right :) |
@deeprobin: Calling it a "performance trap" from which your code "suffers" is a huge exaggeration that can be okay for a YouTube video title. But what it suggests could be applied even for As a real-life scenario, I suggest to add some cases to your benchmark where enum values are part of log messages, for example. Resolving the placeholders in a In fact, |
Hey everyone, just jumping in to give my two cents since I made the video in the first place and I don't wanna be taken out of context. Bit of a background first. I didn't discover this, I actually attended @davidfowl's talk at IO Associates and he mentions this very use-case, the switch being a solution and addresses the O(n) and O(log(n)) thing. All I did is take that, wrap it in a good story and a clickable title and publish it. Now as I mentioned in the video, people don't have to do this until the need to, which basically eliminates the vest majority of C# devs. Do I have usecases for it personally? Sure but that's just another thing in my micro optimization "checklist". So is not using LINQ, eliminating heap allocations when possible, preventing boxing and a bunch of other stuff. Realistically the only thing I wanted out of that video was for someone to take the time and create a source generator that can do this automatically at compile time so I don't have to create switches by hand, and I got that. That's all there is to it. |
I would look into adding a source code generator to the Is there anything against implementing a source code generator for this? |
That does what, specifically? |
AFAIK, Roslyn source generators currently can only add source code, not remove or modify any. So if the original source code contains an enum type definition, then it will be compiled. C# enum type definitions cannot be partial, and according to ECMA-335 6th edition sections I.8.5.2 (Assemblies and scoping) and II.22.37 (TypeDef : 0x02), enum types cannot define methods of their own. So the source generator cannot inject an override ToString() method in the enum type. It could generate a type with an extension method, but if the extension method were named ToString(), then C# would not use it because the enum type inherits Enum.ToString() already. So the best that the source generator could do in the current SDK would be to generate a class with a differently-named extension method, e.g. ToStringFast(), and recommend its use with an analyzer and a code fixer. It might be possible to automatically use this for interpolated strings, though. |
You can't override ToString on an enum in C#, which is what a C# source generator would output. And we can't / wouldn't want to modify the base Enum.ToString to know about every enum in the universe, nor would we want it to use some kind of reflection to find and hook up to some well-known-named method that provided this functionality. Having a heavily-optimized ToString implementation that comes at the expense of a non-trivial amount of additional IL (especially for larger enums) is not something that 99.9% of enums require. As @KalleOlaviNiemitalo outlined, a source generator could output an extension method for a specific enum, based for example on attributing that enum with some attribute the source generator knows about, and any code paths that need the super-fast access could call that extension method. I do not currently see a strong need for such a source generator to be in the .NET SDK; if a source generator here is something you're interested in pursuing, I suggest you do it in some other repo, release it as a nuget for others to consume, etc. Only a small percentage of valuable source generators need actually be in the .NET SDK. |
True. Accordingly, we could have an intrinsic method that prints the name of an enum variant. Or is there something against it? |
I don't understand the suggestion. Can you elaborate? And also share an example scenario where it's important to have the optimization you're suggesting because it's on a really hot path? |
In any case, the implementation does no harm. It is nothing performance critical, but we are all generally interested in improving the performance of .NET. |
To place things in context I created a small test. Of course, when comparing the switched optimization directly to
Please also note that:
Disclaimer: The linked test uses a very short warm-up period to spare the resources of the .NET Fiddle site. To get more reliable results (with low worst-best differences) execute it on your computer with default configuration. The test needs this package (and as I mentioned in my last comment it happens to have some solution for faster enum handling but on recent .NET platforms I would not say it means a huge benefit). |
So I understand this issue doesn't have much love, but just wanted to add something I was experimenting with.
That's exactly what I did in this NuGet Package and it works great 🙂 However, I was interested playing with the interceptors support in .NET 8. So I put together a simple example program:
There's a couple of interesting things here:
I can understand that there are deep CLR reasons why this won't work, but a runtime |
This is a bug in Roslyn compiler. I have opened an issue for it: dotnet/roslyn#70841 |
Microsoft.Extensions.EnumStrings looks related… but it has no 8.0.0 release because of dotnet/extensions#4639. |
* Show confirmation dialog (#221) * Added `showConfirmation` to replace native JS alert using `MessageBox.ts` * Validations and bug fixes * Don't use showConfirmation at deleteFile (modal inside modal) * Missing jjmasterdata.js * Load Additional ConnectionStrings from IOptions (#220) AdditionalConnectionStrings * Encryption fixes * UI and rename `showConfirmation` to `showConfirmationMessage` * Create _ConnectionStringModel.cshtml * Created `jj-list-group` CSS class * Fixed `ExpressionTagHelper` at FloatingLabel * `IconPickerTagHelper` bug fix * _Menu `ms-auto` * Better NCalc extension method with pre-configured options (#223) * Message toast (#224) A simple toast message * Added `GetDataSet` methods to `DataAccess` (#226) * Add `GetDataSet` to `IEntityRepository` * Invert `MessageBox.ts` `showConfirmationMessage` buttons * Fix CodeMirror not looking good at Dark Mode * Added `GetDataSetAsync` * Update docs * Update to NCalc v13 (#230) * Update to NCalc v13 * Added --md-sticky-top-height variable * Change `GetDataSetAsync` CommandBehavior * Delete unused class * Remove unused `DeleteSelectedRowsAction` * `ModelStateWrapper` now allow empty `ModelStateDictionary` * Adjust the log layout * Breadcrumb Component * Added `TypeIdentifier` to `FormElement` * Use Breadcrumb component * Added `Content` init accessor to `BreadcrumbItem` * Breadcrumb development * Support FormElement Identifier * Add default padding to `_MasterDataLayout` * Adjust Horizontal Layout * `ViewBag.IsForm` conditionals * Added `ExpressionDataAccessCommandFactory` * Added ModelState support to `ValidationSummary` taghelper * Added `JJOffcanvas` * Remove dependencies from About * Allow `JJDataPanel` at `PageState.Filter` (#232) * Fix `DbLogger` eventId and added `LogGridViewDataSourceException` * Move Floating Labels to `FormElementOptions` * Added `GridRenderingTemplate` * l10n and missing `JsonProperty` * Fix `PdfWriter.cs` * `loadFieldDetails` bug fix * Fix `list-group-item` at dark theme. * Use `ExpressionHelper` for `GridRenderingTemplate` * Fix `DataItem.Command.Sql` editor * Add `ViewBag.IsForm` to `_FieldLayout.cshtml` * Update Directory.Build.props * fix Settings breadcrumb * bugfix: Add `IFloatingLabelControl` to handle more floating label components * bugfix: Add `small` CSS class to selected items at `GridPagination` * Remove unnecesary async keyword at OffcanvasHelper.ts * Remove unnecessary `SetsRequiredMembersAttribute` * Added `MessageToastTagHelper` and `MessageToast.ts` * Update NCalc to v4 - Now without Antlr! * Update Core.csproj * Added actions to `JJTitle` and allow editing element from /Render * Improve `JJDataPanel` debug by using `Task<List<HtmlBuilder>>` instead of `IAsyncEnumerable<HtmlBuilder>` * bugfix: `DataPanel` not respecting `AutoReloadFormFields` * bugfix: `Modal.ts` not using `getMasterDataForm()` * Improve debug by removing `System.Linq.Async` from `JJMasterData.Commons` * Improve HtmlBuilder performance (dotnet/runtime/issues/57748) * Fix `InsertAtGridView` DataPanel `AutoReloadFormFields` behavior. * Hide `BackAction` when `IsChildFormView` * Remove redundant value * Remove More from `_Menu` * Remove preview from package version and update plugins --------- Co-authored-by: Gustavo Mauricio de Barros <gustavomauriciodebarros@gmail.com>
* Show confirmation dialog (#221) * Added `showConfirmation` to replace native JS alert using `MessageBox.ts` * Validations and bug fixes * Don't use showConfirmation at deleteFile (modal inside modal) * Missing jjmasterdata.js * Load Additional ConnectionStrings from IOptions (#220) AdditionalConnectionStrings * Encryption fixes * UI and rename `showConfirmation` to `showConfirmationMessage` * Create _ConnectionStringModel.cshtml * Created `jj-list-group` CSS class * Fixed `ExpressionTagHelper` at FloatingLabel * `IconPickerTagHelper` bug fix * _Menu `ms-auto` * Better NCalc extension method with pre-configured options (#223) * Message toast (#224) A simple toast message * Added `GetDataSet` methods to `DataAccess` (#226) * Add `GetDataSet` to `IEntityRepository` * Invert `MessageBox.ts` `showConfirmationMessage` buttons * Fix CodeMirror not looking good at Dark Mode * Added `GetDataSetAsync` * Update docs * Update to NCalc v13 (#230) * Update to NCalc v13 * Added --md-sticky-top-height variable * Change `GetDataSetAsync` CommandBehavior * Delete unused class * Remove unused `DeleteSelectedRowsAction` * `ModelStateWrapper` now allow empty `ModelStateDictionary` * Adjust the log layout * Breadcrumb Component * Added `TypeIdentifier` to `FormElement` * Use Breadcrumb component * Added `Content` init accessor to `BreadcrumbItem` * Breadcrumb development * Support FormElement Identifier * Add default padding to `_MasterDataLayout` * Adjust Horizontal Layout * `ViewBag.IsForm` conditionals * Added `ExpressionDataAccessCommandFactory` * Added ModelState support to `ValidationSummary` taghelper * Added `JJOffcanvas` * Remove dependencies from About * Allow `JJDataPanel` at `PageState.Filter` (#232) * Fix `DbLogger` eventId and added `LogGridViewDataSourceException` * Move Floating Labels to `FormElementOptions` * Added `GridRenderingTemplate` * l10n and missing `JsonProperty` * Fix `PdfWriter.cs` * `loadFieldDetails` bug fix * Fix `list-group-item` at dark theme. * Use `ExpressionHelper` for `GridRenderingTemplate` * Fix `DataItem.Command.Sql` editor * Add `ViewBag.IsForm` to `_FieldLayout.cshtml` * Update Directory.Build.props * fix Settings breadcrumb * bugfix: Add `IFloatingLabelControl` to handle more floating label components * bugfix: Add `small` CSS class to selected items at `GridPagination` * Remove unnecesary async keyword at OffcanvasHelper.ts * Remove unnecessary `SetsRequiredMembersAttribute` * Added `MessageToastTagHelper` and `MessageToast.ts` * Update NCalc to v4 - Now without Antlr! * Update Core.csproj * Added actions to `JJTitle` and allow editing element from /Render * Improve `JJDataPanel` debug by using `Task<List<HtmlBuilder>>` instead of `IAsyncEnumerable<HtmlBuilder>` * bugfix: `DataPanel` not respecting `AutoReloadFormFields` * bugfix: `Modal.ts` not using `getMasterDataForm()` * Improve debug by removing `System.Linq.Async` from `JJMasterData.Commons` * Improve HtmlBuilder performance (dotnet/runtime/issues/57748) * Fix `InsertAtGridView` DataPanel `AutoReloadFormFields` behavior. * Hide `BackAction` when `IsChildFormView` * Remove redundant value * Remove More from `_Menu` * Change `ConnectionString` nullability * Use last form at the page to prevent issues with modal * Allow Multiselect at non 'F' `FormElement`s * bugfix: `ExpressionDataAccessCommandFactory` not converting UserId * bugfix: OriginalName form field was creating the fields at `FieldController` * Ignore case at `JJDataPanel` dictionaries * Update jQuery to latest version * Add OffcanvasHelper.hide * R# analyzer as error: Make method static to improve performance * `JJAlert`: Better layout for multiple messages and title * bugfix: Log is with wrong title * bugfix: Custom `DataPanel` with `PageState.Filter` was showing (All) at filter. * bugfix: Encode was happening two times at GridView * bugfix: Sticky was not working * Update version --------- Co-authored-by: Lucio Pelinson <lucio@jjconsulting.com.br> Co-authored-by: Lucio Pelinson <lucio.pelinson@gmail.com>
Description
Since Enum.ToString usually accesses Enum.GetEnumName, which accesses it in the runtime.
runtime/src/libraries/System.Private.CoreLib/src/System/Enum.cs
Lines 1136 to 1146 in 895c99c
runtime/src/libraries/System.Private.CoreLib/src/System/Enum.cs
Lines 135 to 147 in 895c99c
runtime/src/libraries/System.Private.CoreLib/src/System/Enum.cs
Lines 124 to 133 in 895c99c
However, since the name is already given at compile time I think this should be treated as a constant and assembled at compile time.
Configuration
Data
Benchmarks
References
Enum.GetEnumName IL
Extension method IL
We see the IL is bigger, but the code is faster.
Analysis
It is because in runtime the value is resolved although it is already known at compile time.
Perhaps it would also make sense to add a
valuenameof
keyword similar tonameof
with the difference thatvaluenameof
is taken the name of the corresponding enum variant.External references
The text was updated successfully, but these errors were encountered: