maui
maui copied to clipboard
Improve performance of rendering `DatePicker`
Description of Change
- Instead of
DateTime.NowI'm using theDateTimeOffset.Now.DateTime.Date.
| Before | After |
|---|---|
- I've replaced the
.ToUniversalTime()fromUpdateMinimumDate()andUpdateMaximumDate()with.ToUniversalTimeNative()which uses TimeZone info from Java skipping loading the TZ database through .NET
| Before | After |
|---|---|
Issues Fixed
Fixes #24929
This issue also includes TimePicker but I haven't looked into it yet.
Performance change
Recorded in release configuration on physical android device - Samsung Galaxy A50.
| Before | After |
|---|---|
/azp run
Azure Pipelines successfully started running 3 pipeline(s).
Okay, I think I went a bit too far.
Reverting everything and only executing DateTime.Now and DateTime.ToString() on another thread after startup seems fine.
Now just the matter of where should I do this? Unless static constructor of DatePicker.cs is fine.
/azp run
Azure Pipelines successfully started running 3 pipeline(s).
Failing tests probably require a retry.
By the way, some dotnet-trace comparison:
DatePicker.cctor
Before
After
DatePickerExtension.SetText()
Before
After
Full dotnet-trace after this PR
maui-app_20240927_155509.speedscope.json
Only heavy thing left is the CreateDatePickerDialog()
But as Starchm pointed out - removing it will break workaround.
Is this only on Android ? Or better if you do this on iOS do you see performance wins to? @jonathanpeppers any ideas here?
Is this only on Android ? Or better if you do this on iOS do you see performance wins to? @jonathanpeppers any ideas here?
Sadly currently I don't have any Apple devices to test iOS, but if DatePicker for iOS also uses DateTime.Now or DateTime.ToString() then there should be also some improvement.
In general I think initializing like this should improve any case where DateTime.Now or DateTime.ToString() is used (just the first call of course).
Is this only on Android ? Or better if you do this on iOS do you see performance wins to? @jonathanpeppers any ideas here?
Seems like
DateTime.Now/DateTime.Today is in the general class not the platform dependend one so iOS should be a bit better.
And
DatePickerExtension for iOS is also using DateTime.ToString() in some places.
Also what do you guys think about maybe making another PR targeting .NET 9.0 which would remove the CreateDatePickerDialog in DatePicker constructor to make it as smooth as possible?
Since the issue mentioned by Starchm is already fixed then maybe there is no reason to hurt performance to maintain unnecessary workaround?
Also I guess some information about a change breaking this workaround would be needed then.
Also what do you guys think about maybe making another PR targeting .NET 9.0 which would remove the
CreateDatePickerDialoginDatePickerconstructor to make it as smooth as possible? Since the issue mentioned by Starchm is already fixed then maybe there is no reason to hurt performance to maintain unnecessary workaround?Also I guess some information about a change breaking this workaround would be needed then.
The issue got closed with a partial fix as i remember. Fix was done for the iOS part, but not for android. #20547 is still open for droid.
Also what do you guys think about maybe making another PR targeting .NET 9.0 which would remove the
CreateDatePickerDialoginDatePickerconstructor to make it as smooth as possible? Since the issue mentioned by Starchm is already fixed then maybe there is no reason to hurt performance to maintain unnecessary workaround? Also I guess some information about a change breaking this workaround would be needed then.The issue got closed with a partial fix as i remember. Fix was done for the iOS part, but not for android. #20547 is still open for droid.
oh alright I didn't see that sorry. But I guess it still might be worth revisiting after that one is closed.
From the linked issue, it feels like we could instead:
- Remove this line, we can probably lazily create this dialog instead:
https://github.com/dotnet/maui/blob/68524c8056491da6a40b06dd0c979de1bb40538f/src/Core/src/Handlers/DatePicker/DatePickerHandler.Android.cs#L23
^ This could happen when the underlying text box is focused for the first time.
- Investigate if a TZ database is loaded for
DateTime.ToString(CultureInfo.CurrentCulture)and if so, can MAUI useInvariantCultureinstead. There might also be something the BCL can improve on mobile here.
I'm not sure the changes help anything at all here?
- Android puts processor cores to sleep for battery saving purposes. Calling
Task.Run()on startup when in this state will cause a significant delay as the OS has to "wake up" an extra core. This is probably less noticeable on fast/flagship Pixel devices. I wouldn't recommend starting any threads at startup, until your app needs to make a network request, where you can't avoid it.- dotnet/runtime has already implemented something like this in .NET 7+:
The first call to
DateTime.Nowrequires the BCL to load a "timezone database", in order to do the appropriate math for the current time. I would not recommend ever usingDateTime.Now, unless you absolutely have to show the current time on the screen to a user. I would only useDateTime.UtcNowor the equivalentDateTimeOffsetAPIs.So that leaves me with two questions:
- What is the code that is slow? Do we have a
.speedscope?- Is something in .NET MAUI calling
DateTime.Now? Can we remove the call instead?
-
I wasn't sure exactly how it works but I was suspecting possible startup performance decrease, that's why I raised some questions in the linked issue https://github.com/dotnet/maui/issues/24929
-
I was thinking about Utc time but I didn't know about
GetLocalUtcOffsetI will investigate in an hour.
- What is the code that is slow? Do we have a
.speedscope?
Yes, there are 2 speedscopes in linked issue, and 1 somewhere above after the changes.
- Is something in .NET MAUI calling
DateTime.Now? Can we remove the call instead?
Also there are precise locations in linked issue.
From the linked issue, it feels like we could instead:
- Remove this line, we can probably lazily create this dialog instead:
https://github.com/dotnet/maui/blob/68524c8056491da6a40b06dd0c979de1bb40538f/src/Core/src/Handlers/DatePicker/DatePickerHandler.Android.cs#L23
^ This could happen when the underlying text box is focused for the first time.
- Investigate if a TZ database is loaded for
DateTime.ToString(CultureInfo.CurrentCulture)and if so, can MAUI useInvariantCultureinstead. There might also be something the BCL can improve on mobile here.
- I was already going into this direction in the initial version but as Starchm pointed out already - we shouldn't remove that since it's necessary for a workaround for unsolved issue
Removing these lines would probably break a lot of apps, where they use a workaround like https://github.com/dotnet/maui/issues/12899#issuecomment-1403661769 for the datepicker unfocus event.
InvariantCulture- do we want such behaviour? I wouldn't want someone to open a bug report that DatePicker has wrong date format.
I've tried DateTimeOffset
static DateTime GetDefaultDate()
{
return DateTimeOffset.Now.DateTime;
}
And indeed even without initializing TZ DB beforehand it's fast.
But there are still new things appearing, in this case it's this function
https://github.com/dotnet/maui/blob/cc683f0e99547c2ed4875296d7451332ef64ca73/src/Core/src/Platform/Android/DatePickerExtensions.cs#L48-L54
maui-app_20240928_194237.speedscope.json
I could keep trying to get rid of calls which require time zones but still the DateTime.ToString() will remain which I don't see solution for without creating some ugly behaviour.
@jonathanpeppers isn't there by any chance an event that is fired when app is loaded? Maybe it could be used to initialize everything once, solve all possible issues with DateTime across whole app and not affect startup? Or if not an event then maybe it could be fired after some delay from some of the last methods executed on startup?
Looking at this https://github.com/dotnet/maui/blob/cc683f0e99547c2ed4875296d7451332ef64ca73/src/Core/src/Platform/Android/DatePickerExtensions.cs#L48-L54
I'm wondering why is it exactly using ToUniversalTime().
Looking at
and at the definition of
BindableProperty
/// <summary>Bindable property for <see cref="MaximumDate"/>.</summary>
public static readonly BindableProperty MaximumDateProperty = BindableProperty.Create(nameof(MaximumDate), typeof(DateTime), typeof(DatePicker), new DateTime(2100, 12, 31), validateValue: ValidateMaximumDate, coerceValue: CoerceMaximumDate);
Using .ToUniversalTime() seems to be wrong?
Removing it completely makes the date range actually correct with what's passed through BindableProperty and in terms of performance:
this method is no longer problematic.
After those changes maui-app_20240929_002741.speedscope.json Remaining slow things:
DateTime.ToString()CreateDatePickerDialog()inDatePickerconstructor
I think the delay is smaller.
After removing the CreateDatePickerDialog() in the future I think it actually might be acceptable.
Regarding SetText() since it's an extension for android only I think there is a possibility to achieve correct result much faster using native APIs like Java.Text.SimpleDateFormat but since we support all possible C# formats and (I think) C# date formats are not compatible with Java formats making it work might not be worth it?
Trace recorded using some of the native APIs:
/azp run
Azure Pipelines successfully started running 3 pipeline(s).
/azp run
Azure Pipelines successfully started running 3 pipeline(s).
/rebase
Azure Pipelines successfully started running 3 pipeline(s).
Azure Pipelines successfully started running 3 pipeline(s).
Nothing related in failing tests.
/rebase
No response to questions, and a lot changed with DatePicker/TimePicker for .NET 10 so maybe this isn't even an issue anymore. We need to validate that. If this is still of interest to anyone please let us know, closing as stale for now.