This repository has been archived by the owner on May 1, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[android] remove Anticipator.cs for now #8858
Merged
kingces95
merged 1 commit into
xamarin:4.4.0
from
jonathanpeppers:remove-anticipator-for-now
Jan 8, 2020
Merged
[android] remove Anticipator.cs for now #8858
kingces95
merged 1 commit into
xamarin:4.4.0
from
jonathanpeppers:remove-anticipator-for-now
Jan 8, 2020
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
Dec 11, 2019
jonathanpeppers
force-pushed
the
remove-anticipator-for-now
branch
from
December 18, 2019 23:13
eb0bc60
to
50d8939
Compare
rebased to fix merge conflict |
kingces95
reviewed
Jan 7, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not delete Anticipator.cs as I'm editing it and removing file will cause me merge headaches. Feel free to use use preprocessor directives to exclude it if you'd like.
@kingces95 I can use Should this also retarget 4.4.0? I opened this PR before I understood how the branching is done here. |
You've removed the call sites so simplest thing would be to simply undelete the file. |
Context: https://github.com/jonathanpeppers/anticipator Context: xamarin#8245 I was profiling the Blank Xamarin.Forms app template and saw this: Method call summary Total(ms) Self(ms) Calls Method name 40095 0 11 (wrapper runtime-invoke) <Module>:runtime_invoke_void__this___object (object,intptr,intptr,intptr) 40094 0 8 System.Threading.ThreadHelper:ThreadStart (object) 40094 0 8 System.Threading.ExecutionContext:Run (System.Threading.ExecutionContext,System.Threading.ContextCallback,object) 40094 0 8 System.Threading.ExecutionContext:Run (System.Threading.ExecutionContext,System.Threading.ContextCallback,object,bool) 40094 0 8 System.Threading.ExecutionContext:RunInternal (System.Threading.ExecutionContext,System.Threading.ContextCallback,object,bool) 40092 0 8 System.Threading.ThreadHelper:ThreadStart_Context (object) 40092 0 8 Xamarin.Forms.Platform.Android.Anticipator:Loop (object) 40035 0 16 System.Threading.WaitHandle:WaitOne (System.TimeSpan) 40035 0 16 System.Threading.WaitHandle:WaitOne (System.TimeSpan,bool) 40035 0 16 System.Threading.WaitHandle:WaitOne (long,bool) 40035 0 16 System.Threading.WaitHandle:InternalWaitOne (System.Runtime.InteropServices.SafeHandle,long,bool,bool) 40034 0 16 System.Threading.WaitHandle:WaitOneNative (System.Runtime.InteropServices.SafeHandle,uint,bool,bool) 40034 40034 16 (wrapper managed-to-native) System.Threading.WaitHandle:Wait_internal (intptr*,int,bool,int) The `Anticipator` class stuck out, so I profiled it separately in a blank app (not using Xamarin.Forms at all). This project isolates the `Anticipator`, in timing specifically `Build.VERSION.SdkInt` and the `Resource.designer.cs` class. This launches an app 10 times and averages the result. The results were that it hurt performance in every scenario: HAXM Emulator / Debug Anticipator: 141.273 No Anticipator: 108.182 HAXM Emulator / Release Anticipator: 86.182 No Anticipator: 63.545 Pixel 3XL / Debug Anticipator: 101.545 No Anticipator: 93.182 Pixel 3XL / Release Anticipator: 45.818 No Anticipator: 38.455 Pixel 3XL / Release+AOT Anticipator: 33.636 No Anticipator: 30.545 Feel free to try out my sample as additional verification. I would recommend removing this class for now, and only bring it back when we have some numbers showing it improves things. Sorry!
jonathanpeppers
force-pushed
the
remove-anticipator-for-now
branch
from
January 7, 2020 22:12
50d8939
to
1bcd7a2
Compare
kingces95
approved these changes
Jan 8, 2020
samhouts
added
the
t/housekeeping ♻︎
Internal only changes, won't be included in release notes
label
Jan 10, 2020
tuyen-vuduc
pushed a commit
to NAXAM/Xamarin.Forms
that referenced
this pull request
Jan 14, 2020
* 'master' of github.com:xamarin/Xamarin.Forms: (330 commits) [Android] Fix color filter usage on API29 (xamarin#9180) Add null check to GetIconColor (xamarin#9172) Apply cecil fixes to make packages 2017 compatible (xamarin#9145) [UWP] Attempt to resolve entry on UWP not correctly calculating the correct height when in a scroll view (xamarin#8214) Fixes 7992 Changes UWP DatePicker to show the picker flyout on DatePickerFocus() (xamarin#8056) Update bug_report.md (xamarin#8688) [android/ios] improve perf when not using Application.Properties (xamarin#8887) The great Androidx IF Def'ing of 2019 (xamarin#8898) Added IconColor property for managing navigation icon color (xamarin#5185) Add UWP display prompt (xamarin#8720) fix bad merge Fix 8743 - now using specific style in SearchBar [UWP] (xamarin#8773) Added the SwipeView tag to the Core Gallery samples (xamarin#8819) [Tizen] Shell: FlyoutBackgroundImage, FlyoutBackgroundImageAspect (xamarin#8905) [Core] remove array covariant cast for UWP (xamarin#9135) [android] remove Anticipator.cs for now (xamarin#8858) Update the CarouselView Position setting the CurrentItem (xamarin#7946) fixes xamarin#7924 send remove events (xamarin#9124) [platform] improve perf of PropertyChangedEventArgsExtensions (xamarin#9084) Fix SeachBarRenderer CreateNativeControl issue (xamarin#8946) ... # Conflicts: # Xamarin.Forms.Core/FontImageSource.cs
jonathanpeppers
added a commit
to jonathanpeppers/Xamarin.Forms
that referenced
this pull request
Jan 16, 2020
Context: xamarin#8858 After merging from 4.4.0, etc. this class came back. @kingces95 did not want to remove the `Anticipator.cs` file. Since `Xamarin.Forms.Platform.Android.csproj` is now a short-form MSBuild project, you have to add an explicit: `<Compile Remove="Anticipator.cs"/>` to remove it.
2 tasks
samhouts
pushed a commit
that referenced
this pull request
Jan 17, 2020
Context: #8858 After merging from 4.4.0, etc. this class came back. @kingces95 did not want to remove the `Anticipator.cs` file. Since `Xamarin.Forms.Platform.Android.csproj` is now a short-form MSBuild project, you have to add an explicit: `<Compile Remove="Anticipator.cs"/>` to remove it.
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
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.
Description of Change
Context: https://github.com/jonathanpeppers/anticipator
Context: #8245
I was profiling the Blank Xamarin.Forms app template and saw this:
The
Anticipator
class stuck out, so I profiled it separately in ablank app (not using Xamarin.Forms at all). This project isolates the
Anticipator
, in timing specificallyBuild.VERSION.SdkInt
and theResource.designer.cs
class. This launches an app 10 times andaverages the result.
The results were that it hurt performance in every scenario:
Feel free to try out my sample as additional verification.
I would recommend removing this class for now, and only bring it back
when we have some numbers showing it improves things. Sorry!
Let me know if I need to target a branch other than master, thanks!
Issues Resolved
n/a
API Changes
None
Platforms Affected
Behavioral/Visual Changes
None
Before/After Screenshots
Not applicable, I think
Testing Procedure
Feel free to the project here, for additional verification: https://github.com/jonathanpeppers/anticipator
I will work on getting a script like this in Xamarin.Forms itself, documentation, etc.
PR Checklist