Skip to content
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

Fix DatePicker slow first render (#24929) #24948

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

OvrBtn
Copy link

@OvrBtn OvrBtn commented Sep 26, 2024

Context

I'd say that this solution doesn't look nice but at the same time I don't have other ideas and since it works I'm creating this PR to show my solution.
By the way, I'm completely new to contributing so I'm sorry if something is wrong :)

Description of Change

Based on information from #24929 I've done:

  1. DateTime.Now and DateTime.ToString() initialization on startup currently in static constructor, later it probably should be moved somewhere more appropriate.
  2. DateTime.Now and DateTime.ToString() initialization on worker thread in case someone uses DatePicker on app's main page.
  3. I've removed creating DatePickerDialog on DatePicker constructor since it's also slowing down first render (see dotnet trace available in linked issue), instead it's created only on demand.

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
Screen_Recording_20240926-193904.mp4
Screen_Recording_20240926-193355.mp4

@OvrBtn OvrBtn requested a review from a team as a code owner September 26, 2024 18:22
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Sep 26, 2024
@jfversluis
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@OvrBtn OvrBtn changed the title Fix DateTime slow first render (#24929) Fix DatePicker slow first render (#24929) Sep 26, 2024
@OvrBtn
Copy link
Author

OvrBtn commented Sep 27, 2024

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.

Screen_Recording_20240927-030030.mp4

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@OvrBtn
Copy link
Author

OvrBtn commented Sep 27, 2024

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.

@rmarinho
Copy link
Member

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

@OvrBtn
Copy link
Author

OvrBtn commented Sep 27, 2024

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
Copy link
Author

OvrBtn commented Sep 27, 2024

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
Copy link
Author

OvrBtn commented Sep 27, 2024

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.

@Starchm
Copy link

Starchm commented Sep 27, 2024

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.

@OvrBtn
Copy link
Author

OvrBtn commented Sep 27, 2024

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.

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@jonathanpeppers
Copy link
Member

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

  1. Remove this line, we can probably lazily create this dialog instead:

_dialog = CreateDatePickerDialog(date.Value.Year, date.Value.Month, date.Value.Day);

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

@OvrBtn
Copy link
Author

OvrBtn commented Sep 27, 2024

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 [Performance] DatePicker/TimePicker first render is too slow #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
Copy link
Author

OvrBtn commented Sep 27, 2024

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

  1. Remove this line, we can probably lazily create this dialog instead:

_dialog = CreateDatePickerDialog(date.Value.Year, date.Value.Month, date.Value.Day);

^ 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 #12899 (comment) 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
Copy link
Author

OvrBtn commented Sep 28, 2024

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

public static void UpdateMaximumDate(this MauiDatePicker platformDatePicker, IDatePicker datePicker, DatePickerDialog? datePickerDialog)
{
if (datePickerDialog != null)
{
datePickerDialog.DatePicker.MaxDate = (long)datePicker.MaximumDate.ToUniversalTime().Subtract(DateTime.MinValue.AddYears(1969)).TotalMilliseconds;
}
}

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
Copy link
Author

OvrBtn commented Sep 28, 2024

Looking at this

public static void UpdateMaximumDate(this MauiDatePicker platformDatePicker, IDatePicker datePicker, DatePickerDialog? datePickerDialog)
{
if (datePickerDialog != null)
{
datePickerDialog.DatePicker.MaxDate = (long)datePicker.MaximumDate.ToUniversalTime().Subtract(DateTime.MinValue.AddYears(1969)).TotalMilliseconds;
}
}

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
Copy link
Author

OvrBtn commented Sep 28, 2024

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

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

I think the delay is smaller.

Screen_Recording_20240928-234929.mp4

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

@OvrBtn
Copy link
Author

OvrBtn commented Sep 29, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Performance] DatePicker/TimePicker first render is too slow
6 participants