[android] improve performance of Entry.MaxLength
#15614
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonathanpeppers
commented
Jun 13, 2023
| if (maxLength == -1) | ||
| maxLength = Integer.MAX_VALUE; | ||
|
|
||
| List<InputFilter> currentFilters = new ArrayList<>(Arrays.asList(editText.getFilters())); |
There was a problem hiding this comment.
In C#, we had a null check for getFilters() (?? new IInputFilter[0]), but I don't think it can return null:
I can't find any null checks for it in Android's source code:
https://github.com/search?q=repo%3Aaosp-mirror%2Fplatform_frameworks_base%20getFilters&type=code
Editor is a good example:
jonathanpeppers
added
platform/android 🤖
legacy-area-perf
jonathanpeppers
commented
Jun 16, 2023
Comment on lines
-49
to
-50
| if (e.BeforeCount == 0 && e.AfterCount == 0) | ||
| return; |
There was a problem hiding this comment.
In 85b5699, it doesn't say why this check was added, but it was causing test failures when MaxLength is set to 0.
If you had an Entry with text, then set MaxLength=0 it wasn't updating Entry.Text after the platform fired the TextChanged event.
jonathanpeppers
commented
Jun 16, 2023
| @@ -358,6 +359,7 @@ public async Task NegativeMaxLengthDoesNotClip() | |||
| , Skip = "https://github.com/dotnet/maui/issues/7939" | |||
| #endif | |||
| )] | |||
| [InlineData(0)] | |||
There was a problem hiding this comment.
This test case was failing without the above change to ITextInputExtensions.
|
/rebase |
Context: dotnet#10713 When investigating a customer sample: * Navigate from a Shell flyout * To a new page with several `Entry` controls When profiling on a Pixel 5, it seems one hot path is `Entry.MaxLength`: 18.52ms (0.22%) microsoft.maui!Microsoft.Maui.Platform.EditTextExtensions.UpdateMaxLength(Android.Widget.EditText,Microsoft.Maui.IEntry) 16.03ms (0.19%) microsoft.maui!Microsoft.Maui.Platform.EditTextExtensions.UpdateMaxLength(Android.Widget.EditText,int) 12.16ms (0.14%) microsoft.maui!Microsoft.Maui.Platform.EditTextExtensions.SetLengthFilter(Android.Widget.EditText,int) * `EditTextExtensions.UpdateMaxLength()` calls * `EditText.Text` getter and setter * `EditTextExtensions.SetLengthFilter()` calls * `EditText.Get/SetFilters()` What happens is we end up marshaling strings and `IInputFilter[]` back and forth between C# and Java for every `Entry` on the page. This seems like a prime candidate to move code from C# to Java. I tried to just port the code as-is without changing logic -- so we *should* get the exact same behavior as before. Since all `Entry`s go through this code path (even ones with a default value for `MaxLength`), this improves the performance of all `Entry` and `SearchBar` on Android. With these changes in place, the calls to `EditTextExtensions.UpdateMaxLength()` are now so fast they are missing from the trace now, saving ~19ms when navigating to this page. One note, is I found this was the recommended way to create a mutable `List<T>` from an array in Java: List<InputFilter> currentFilters = new ArrayList<>(Arrays.asList(editText.getFilters())); https://stackoverflow.com/a/11659198 Makes me appreciate C#! :) Other changes: * All platform to update empty strings In 85b5699, it doesn't say why this check was added, but it was causing test failures when MaxLength is set to 0.
jonathanpeppers
force-pushed
the
AndroidEntryMaxLength
branch
from
ff65e09 to
7199ff8
Compare
|
I manually rebased this. I don't know how a bot could resolve a conflict in |
mattleibow
approved these changes
Jul 5, 2023
Eilon
added
t/perf
samhouts
added
the
fixed-in-8.0.0-preview.7.8842
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Context: #10713
When investigating a customer sample:
Navigate from a Shell flyout
To a new page with several
EntrycontrolsIt feels a bit slow & laggy
When profiling on a Pixel 5, it seems one hot path is
Entry.MaxLength:EditTextExtensions.UpdateMaxLength()callsEditText.Textgetter and setterEditTextExtensions.SetLengthFilter()callsEditText.Get/SetFilters()What happens is we end up marshaling strings and
IInputFilter[]back and forth between C# and Java for everyEntryon the page.This seems like a prime candidate to move code from C# to Java. I tried to just port the code as-is without changing logic -- so we should get the exact same behavior as before.
Since all
Entrys go through this code path (even ones with a default value forMaxLength), this improves the performance of allEntryandSearchBaron Android. With these changes in place, the calls toEditTextExtensions.UpdateMaxLength()are now so fast they are missing from the trace now, saving ~19ms when navigating to this page.One note, is I found this was the recommended way to create a mutable
List<T>from an array in Java:https://stackoverflow.com/a/11659198
Makes me appreciate C#! 😄