Fix performance for File.GetLastWriteTimeUtc on Unix
#53070
Conversation
All this changes are to internal methods which are later called by other public methods which correctly call `ToLocalTime` for local time "overloads". Performance hit is most visible for application which doesn't need to load local timezone information which takes ~30ms on 2016 MacBook Pro.
|
Tagging subscribers to this area: @carlossanlop Issue DetailsAll this changes are to internal methods which are later called by other public methods which correctly call I tried to create unit tests for
|
|
Hi @DavidKarlas Could you please contribute benchmarks that excercise the methods that you are trying to optimize to dotnet/performance and use corerun to show us the exact difference? |
| EnsureCachesInitialized(path, continueOnError); | ||
| if (!_exists) | ||
| return DateTimeOffset.FromFileTime(0); | ||
| return new DateTimeOffset(DateTime.FromFileTimeUtc(0)); |
There was a problem hiding this comment.
Isn't this a breaking change? Previously the DateTime that was returned would have had a Kind == Local, but now it's going to have a Kind == Utc.
There was a problem hiding this comment.
Ignore that... I was reading it as DateTime rather than DateTimeOffset.
There was a problem hiding this comment.
But... it's still a semantic change... won't the returned DateTimeOffset now have a different value than it did previously, with a different offset? Or it doesn't matter because this method is only used internally and the call sites ensure the right thing happens?
There was a problem hiding this comment.
This is called from 2 locations:
- FileSystemEntry.CreationTimeUtc which by it's name already expects
DateTimeOffsetto beUtc, this is what added unit test ensures. - FileSystemInfo.CreationTimeCore which is only called from FileSystemInfo.CreationTimeUtc, notice that FileSystemInfo.CreationTime calls FileSystemInfo.CreationTimeUtc.ToLocalTime()
I assumed FileSystemInfo.CreationTimeCore on Windows also returns UTC, but while investigating for this answer I noticed that it calls FILE_TIME.ToDateTimeOffset which is also performing ToLocalTime which I think is as unnecessary on Windows as on Unix.
I will change Windows as well and write Performance test as requested by @adamsitnik.
I will add performance tests thank you for pointing that out... I will work on this later this week. |
|
@DavidKarlas I merged from |
|
Thank you for doing that, sorry I dropped a ball a bit on this... I ran into a bit a problem few weeks back... MicroBenchmarks cant capture this fix since they have warmup and repetition, because there isnt much saving there, savings are on first run of app(which is important for CLI tools), because with this change it doesnt need to load Timezones data. |
|
No worries, @DavidKarlas! That's tricky; I'm not sure how to capture good data for that. I'll have to rely on @adamsitnik's signoff/guidance. |
|
Maybe I just need Scenario test, but there arent many examples, 30ms difference is very noticable... |
|
@adamsitnik Could you review this before our Preview 7 snap please? |
jozkee
left a comment
jozkee
left a comment
There was a problem hiding this comment.
This change makes sense to me, should we prioritize it for 6.0? Seems to me that only missing requirement was benchmark results for warmup time, is that correct? Even without the results, it looks reasonable to avoid caching something when is not really needed.
|
@adamsitnik This PR is assigned to you for follow-up/decision before the RC1 snap. @jozkee, feel free to self-assign if you'd like to drive it instead though. |
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, big thanks for the PR and appologies for the delay @DavidKarlas
|
Thank you all for reviewing and merging <3 |
ghost
locked as resolved and limited conversation to collaborators
All this changes are to internal methods which are later called by other public methods which correctly call
ToLocalTimefor local time "overloads".Performance hit is most visible for application which doesn't need to load local timezone information which takes ~30ms on 2016 MacBook Pro.
I tried to create unit tests for
File.GetLastWriteTime/Utc, but problem is that values get converted correctly.Test that I added has a problem, it will pass even if logic regresses on machine with timezone+0.