maui icon indicating copy to clipboard operation
maui copied to clipboard

Improve performance of rendering `DatePicker`

Open OvrBtn opened this issue 1 year ago • 27 comments

Description of Change

  1. Instead of DateTime.Now I'm using the DateTimeOffset.Now.DateTime.Date.
Before After
image image
  1. I've replaced the .ToUniversalTime() from UpdateMinimumDate() and UpdateMaximumDate() with .ToUniversalTimeNative() which uses TimeZone info from Java skipping loading the TZ database through .NET
Before After
image image

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

OvrBtn avatar Sep 26 '24 18:09 OvrBtn

/azp run

jfversluis avatar Sep 26 '24 18:09 jfversluis

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Sep 26 '24 18:09 azure-pipelines[bot]

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.

OvrBtn avatar Sep 27 '24 01:09 OvrBtn

/azp run

rmarinho avatar Sep 27 '24 10:09 rmarinho

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Sep 27 '24 10:09 azure-pipelines[bot]

Failing tests probably require a retry.

By the way, some dotnet-trace comparison:

DatePicker.cctor

Before image After image

DatePickerExtension.SetText()

Before image After image

Full dotnet-trace after this PR maui-app_20240927_155509.speedscope.json

Only heavy thing left is the CreateDatePickerDialog() image But as Starchm pointed out - removing it will break workaround.

OvrBtn avatar Sep 27 '24 14:09 OvrBtn

Is this only on Android ? Or better if you do this on iOS do you see performance wins to? @jonathanpeppers any ideas here?

rmarinho avatar Sep 27 '24 15:09 rmarinho

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).

OvrBtn avatar Sep 27 '24 15:09 OvrBtn

Is this only on Android ? Or better if you do this on iOS do you see performance wins to? @jonathanpeppers any ideas here?

image Seems like DateTime.Now/DateTime.Today is in the general class not the platform dependend one so iOS should be a bit better.

image And DatePickerExtension for iOS is also using DateTime.ToString() in some places.

OvrBtn avatar Sep 27 '24 15:09 OvrBtn

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.

OvrBtn avatar Sep 27 '24 16:09 OvrBtn

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.

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.

Starchm avatar Sep 27 '24 17:09 Starchm

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.

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.

OvrBtn avatar Sep 27 '24 17:09 OvrBtn

From the linked issue, it feels like we could instead:

  1. 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.

  1. Investigate if a TZ database is loaded for DateTime.ToString(CultureInfo.CurrentCulture) and if so, can MAUI use InvariantCulture instead. There might also be something the BCL can improve on mobile here.

jonathanpeppers avatar Sep 27 '24 18:09 jonathanpeppers

I'm not sure the changes help anything at all here?

  1. 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.
  2. dotnet/runtime has already implemented something like this in .NET 7+:

The first call to DateTime.Now requires the BCL to load a "timezone database", in order to do the appropriate math for the current time. I would not recommend ever using DateTime.Now, unless you absolutely have to show the current time on the screen to a user. I would only use DateTime.UtcNow or the equivalent DateTimeOffset APIs.

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?
  1. 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

  2. I was thinking about Utc time but I didn't know about GetLocalUtcOffset I 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.

OvrBtn avatar Sep 27 '24 18:09 OvrBtn

From the linked issue, it feels like we could instead:

  1. 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.

  1. Investigate if a TZ database is loaded for DateTime.ToString(CultureInfo.CurrentCulture) and if so, can MAUI use InvariantCulture instead. There might also be something the BCL can improve on mobile here.
  1. 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.

  1. InvariantCulture - do we want such behaviour? I wouldn't want someone to open a bug report that DatePicker has wrong date format.

OvrBtn avatar Sep 27 '24 18:09 OvrBtn

I've tried DateTimeOffset

static DateTime GetDefaultDate()
{
	return DateTimeOffset.Now.DateTime;
}

image 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 image 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?

OvrBtn avatar Sep 28 '24 19:09 OvrBtn

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 image 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: image this method is no longer problematic.

OvrBtn avatar Sep 28 '24 21:09 OvrBtn

After those changes maui-app_20240929_002741.speedscope.json Remaining slow things:

  • DateTime.ToString()
  • CreateDatePickerDialog() in DatePicker constructor

I think the delay is smaller.

After removing the CreateDatePickerDialog() in the future I think it actually might be acceptable.

OvrBtn avatar Sep 28 '24 22:09 OvrBtn

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: image

OvrBtn avatar Sep 29 '24 01:09 OvrBtn

/azp run

PureWeen avatar Oct 03 '24 21:10 PureWeen

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Oct 03 '24 21:10 azure-pipelines[bot]

/azp run

PureWeen avatar Oct 03 '24 21:10 PureWeen

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Oct 03 '24 21:10 azure-pipelines[bot]

/rebase

jfversluis avatar Oct 17 '24 11:10 jfversluis

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Oct 17 '24 11:10 azure-pipelines[bot]

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Oct 18 '24 18:10 azure-pipelines[bot]

Nothing related in failing tests.

OvrBtn avatar Oct 23 '24 14:10 OvrBtn

/rebase

PureWeen avatar Aug 23 '25 12:08 PureWeen

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.

jfversluis avatar Sep 09 '25 13:09 jfversluis