-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Simplify constant TimeSpan
retrieval
#118097
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
base: main
Are you sure you want to change the base?
Conversation
`TimeSpan.Parse` may behave differently depending on culture settings.
This may be true, but the original version also only ran parameter validation/construction once. |
...es/System.Transactions.Local/src/System/Transactions/Configuration/DefaultSettingsSection.cs
Show resolved
Hide resolved
...es/System.Transactions.Local/src/System/Transactions/Configuration/MachineSettingsSection.cs
Show resolved
Hide resolved
It would be beneficial to share some diffs and/or benchmarks from this. This looks like a pure improvement, but having the numbers just helps add the extra +1 to give people peace of mind. |
Diffs are an improvement, as expected. |
Yep. Mostly looks like removing the static constructors (which should happen in T1 anyways) and that we do get the direct constant as expected. |
Tagging subscribers to this area: @roji, @SamMonoRT |
This LGTM, but since its in a less touched area I will defer to the area owners on whether it meets the bar for merge: @SamMonoRT, @roji |
The JIT compiler can treat
TimeSpan.FromMinutes(1)
as a constant and optimize it at compile time. In contrast,TimeSpan.Parse
involves runtime string parsing and cannot be constant-folded, which adds unnecessary overhead.TimeSpan.Parse
may also behave differently depending on culture settings.