-
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
Add new APIs for abstracting the system clock and local time zone. #48681
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
The proposed API needs to go through a couple of phases before it gets to the PR stage. |
@mattjohnsonpint in #36617 it was discussed that we don't want to allow mocking APIs in the core and that should be supported in the extensions if we need to after getting rid of duplicates. So, I don't think we'll accept such changes before finalizing the discussion. I am adding here the folks who attended the design review for this issue: @GrabYourPitchforks @terrajobst @bartonjs |
Agreed about needing to go through more discussion and phases. I provide the PR so you could see how such an API might be implemented. We'll continue accordingly. On the point of mocking, please see my comments in the issue thread. Thanks. |
Thanks. Closing the PR as the commits can be viewed without needing a PR open. |
If I am reading the benchmark correctly, it will be non-trivial regression. we have been working to enhance the perf of the Now/UtcNow trying to get every gain as possible. The regression percentage is not trivial. don't look at the time units but look at the percentage and consider the scenarios that the app is calling Now/UtcNow more frequent. We are trying to think in ways to enhance the performance and such change would make that difficult. |
I think this API is important but I'm not a fan of the async local usage |
@tarekgh, I saw several things that could improve perf while I was working on this. Some are in here, but several were unrelated to this feature. I can send a separate PR for those things if you like. Generally agree about the perf. Faster is always better. Not sure that a few nanoseconds difference is really all that much though. I did find it odd that the benchmarks showed the |
Please do 😄 |
Would love to find a better way than |
This PR is a proposed implementation of the revised API proposal I submitted for #36617. I do not expect this PR to be merged at this time. Please see the issue thread for details.
One open question I have with specific regards to the implementation (not the public API)
In
TimeContext.cs
, I implement the internal storage for the ambient context simply as:This seems to work fine, and all tests pass. However, looking at the implementation for a similar concept, I see that
CultureInfo.CurrentCulture
andCultureInfo.CurrentUICulture
each use both anAsyncLocal<T>
field and a[ThreadStatic]
field, with some logic synchronizing them. Is that necessary here also? Why or why not?Thanks.
Benchmarks
I ran the existing microbenchmarks from here, that test
DateTime.UtcNow
,DateTimeOffset.UtcNow
,DateTime.Now
, andDateTimeOffset.Now
, both before and after my changes, and both with and without leap seconds enabled (via registry flag as described here). The benchmark results show roughly a 8-20ns cost for theUtcNow
properties, and a 19-92ns cost for theNow
properties, both of which I believe are reasonable and negligible.Full benchmark results follow.
ResultsComparer difference, with leap seconds enabled
ResultsComparer difference, with leap seconds disabled
Test Setup
Before changes, leap seconds enabled
Benchmark Test Class:
System.Tests.Perf_DateTime
Benchmark Test Class:
System.Tests.Perf_DateTimeOffset
After changes, leap seconds enabled
Benchmark Test Class:
System.Tests.Perf_DateTime
Benchmark Test Class:
System.Tests.Perf_DateTimeOffset
Before changes, leap seconds disabled
Benchmark Test Class:
System.Tests.Perf_DateTime
Benchmark Test Class:
System.Tests.Perf_DateTimeOffset
After changes, leap seconds disabled
Benchmark Test Class: System.Tests.
Perf_DateTime
Benchmark Test Class:
System.Tests.Perf_DateTimeOffset