Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[release/7.0] [Android][libs] Introduce DateTimeOffset.Now temporary fast result #74965
Changes from 17 commits
2af813a
2b068d0
1cdcdc1
188acf1
5a2197e
f4b3963
1f37efe
1b1b61a
8a928df
7e67389
f85b518
16ea40f
564f523
8f4abd0
2520363
821d87c
c0a6c24
1088724
a9034d2
eb5a67c
21231f6
e611215
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.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'm curious why this changed?
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 can be a perf win if we are able to delay loading ICU until after startup. This change doesn't guarantee that, it just helps.
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 would StartsWith("GMT", StringComparison.Ordinal) load ICU?
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.
Apparently, it's fairly easy to get to
runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
Lines 19 to 33 in 4da3eae