-
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
[release/7.0] [Android][libs] Introduce DateTimeOffset.Now temporary fast result #74965
Conversation
This reverts commit 1edaeee.
Leverage a boolean flag instead of && Start the background thread to avoid deadlocking
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. |
@eerhardt or @lambdageek can we get a code review sign off? |
@marek-safar Can you please review / approve? |
// prevent two calls to DateTimeOffset.Now in quick succession | ||
// from both reaching here. | ||
s_loadAndroidTZData.Start(); | ||
s_startNewBackgroundThread = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What prevents the race condition where two threads both read s_startNewBackgroundThread as true? Seems like it'd be safer to have a local that determines whether this thread is the one that gets to start it, and that local is set appropriately inside of the above lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we made a change based on feedback from @grendello. I don't recall the specifics as to why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feedback was about the previous state of the code, where the thread was started within the same lock that the thread then takes, which could end in a deadlock. The assumption here is that since .Start()
doesn't block the current thread, the flag is set likely before the thread actually is started. Even in the unlikely situation when two threads would attempt to start the worker thread, the only harm would be a little time wasted inside the worker thread. This code could use a semaphore, for instance, but we felt that would be an overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even in the unlikely situation when two threads would attempt to start the worker thread, the only harm would be a little time wasted inside the worker thread
Calling Start on a thread that's already started will result in an exception. So the concern I'm raising above is that this could lead to reliability problems, not just performance ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a very unlikely event, but sure, it is better to wrap it in in try/catch
then and ignore the potential exception. Performance was the main point of this PR, so it would be a shame to sacrifice it to handle a very unlikely case with semaphores or similar mechanisms.
src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.Android.cs
Outdated
Show resolved
Hide resolved
lock (s_localUtcOffsetLock) | ||
{ | ||
s_loadAndroidTZData = null; // Ensure thread is cleared when cache is loaded | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why store the thread at all? You can use s_startNewBackgroundThread or equivalent for all coordination between threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in response to review on the original PR, here.
src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.Android.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.Android.cs
Outdated
Show resolved
Hide resolved
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsBackport of #74459 to release/7.0 /cc @mdh1418 Customer ImpactSignificantly reduces the performance impact of DateTimeOffset.Now at first call to an avg TestingTested locally using speedscope to ensure that there was a significant performance improvement. Also tested scenarios in which multiple DateTimeOffset.Now calls are made to ensure locks/background threads worked properly with resilience to races and deadlocking. Testing locally with the dotnet/runtime Android sample with this program
The average time of the first call to DateTimeOffset.Now was 21.818ms RiskLow, only impacts DateTimeOffset.Now on Android. IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.
|
Adding the label until the feedback is resolved and CI confirmed to be unrelated. Please ping me when ready so I can merge it. |
src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.Android.cs
Show resolved
Hide resolved
ddb4075
to
eb5a67c
Compare
src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.Android.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.Android.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.Android.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.Android.cs
Show resolved
Hide resolved
@carlossanlop this should be good to go once CI finishes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my feedback.
(We should make sure to keep main up to date with the changes made here.) |
dotnet#74965 contained improvements for the routine. This change brings them back to main.
Backport of #74459 to release/7.0
/cc @mdh1418
Customer Impact
Significantly reduces the performance impact of DateTimeOffset.Now at first call to an avg
21.818ms
(Issue reported 277ms).Testing
Tested locally using speedscope to ensure that there was a significant performance improvement. Also tested scenarios in which multiple DateTimeOffset.Now calls are made to ensure locks/background threads worked properly with resilience to races and deadlocking.
Testing locally with the dotnet/runtime Android sample with this program
The average time of the first call to DateTimeOffset.Now was 21.818ms
The average time of the next call to DateTimeOffset.Now after the background thread finished was 27.821ms
The issue's reported timing of DateTimeOffset.Now was 277ms
Risk
Low, only impacts DateTimeOffset.Now on Android.
IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.