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

Fixed a redundant check that caused a massive slowdown on UWP. #788

Merged
merged 4 commits into from Mar 9, 2017

Conversation

Projects
None yet
5 participants
@BradChase2011
Contributor

BradChase2011 commented Feb 27, 2017

Description of Change

Our views on UWP can take anywhere from 30 seconds to 10 minutes to render. This fix takes all those views down to milliseconds. I would rather have just removed all the IsInNativeLayout and also the MaybeInvalidate() function because they seem to not really do anything. InvalidateLayout shouldnt cause a layout cycle unless something else was wrong. So even though we are checking the tree once per control now instead of multiple times, I believe its still too much and shouldnt be needed.

No Tests needed as this is for performance only. I did see some performance functions in the master code if that is something that can be used on UWP then it would be interesting to see the differences on complex trees.

Bugs Fixed

#52507

API Changes

None

Behavioral Changes

UWP views should load extremely fast now, especially in release.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Feb 27, 2017

@BradChase2011,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

dnfclas commented Feb 27, 2017

@BradChase2011,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@StephaneDelcroix StephaneDelcroix requested a review from jassmith Feb 27, 2017

@jassmith

This comment has been minimized.

Show comment
Hide comment
@jassmith

jassmith Mar 2, 2017

Member

indenting should probably be fixed

Member

jassmith commented Mar 2, 2017

indenting should probably be fixed

Show outdated Hide outdated Xamarin.Forms.Platform.WP8/VisualElementTracker.cs
if (Model.IsInNativeLayout)
return;
var parent = (FrameworkElement)Element.Parent;
if (Model.IsInNativeLayout)

This comment has been minimized.

@rmarinho

rmarinho Mar 2, 2017

Member

can you please fix these changes

@rmarinho

rmarinho Mar 2, 2017

Member

can you please fix these changes

This comment has been minimized.

@BradChase2011

BradChase2011 Mar 2, 2017

Contributor

Yea I had a similar issue on my other PR. I cant seem to figure out why the tabs are not working correctly. I have pulled down a fresh copy on two computers and the tabs are correct on my side when I pull. Any ideas?

@BradChase2011

BradChase2011 Mar 2, 2017

Contributor

Yea I had a similar issue on my other PR. I cant seem to figure out why the tabs are not working correctly. I have pulled down a fresh copy on two computers and the tabs are correct on my side when I pull. Any ideas?

This comment has been minimized.

@jassmith

jassmith Mar 2, 2017

Member

Your editor is inserting spaces not tabs. The new PR still uses spaces.

@jassmith

jassmith Mar 2, 2017

Member

Your editor is inserting spaces not tabs. The new PR still uses spaces.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 2, 2017

Member

Just make a new branch from master, and just add your fix.

Member

rmarinho commented Mar 2, 2017

Just make a new branch from master, and just add your fix.

@BradChase2011

This comment has been minimized.

Show comment
Hide comment
@BradChase2011

BradChase2011 Mar 2, 2017

Contributor

The issue is it will end up doing the same thing as I am not sure why it's happening because on my side it looks correct and I even counted them. Did the web edits fix it?

Anyway I would love to figure it out so I can fix some other stuff when I have time.

Contributor

BradChase2011 commented Mar 2, 2017

The issue is it will end up doing the same thing as I am not sure why it's happening because on my side it looks correct and I even counted them. Did the web edits fix it?

Anyway I would love to figure it out so I can fix some other stuff when I have time.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 3, 2017

Member

@BradChase2011 what IDE are you using?

Member

rmarinho commented Mar 3, 2017

@BradChase2011 what IDE are you using?

@BradChase2011

This comment has been minimized.

Show comment
Hide comment
@BradChase2011

BradChase2011 Mar 3, 2017

Contributor

VS1015, nothing weird.

Contributor

BradChase2011 commented Mar 3, 2017

VS1015, nothing weird.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 3, 2017

Member

img

Can you check if your options are like this? Options / TextEditor / C# / Tabs

Member

rmarinho commented Mar 3, 2017

img

Can you check if your options are like this? Options / TextEditor / C# / Tabs

@BradChase2011

This comment has been minimized.

Show comment
Hide comment
@BradChase2011

BradChase2011 Mar 3, 2017

Contributor

@rmarinho YUP!! That was it, something switched me over to Insert Spaces. UGHH!!! What is weird is I am hitting tab but that setting must switch them out automatically on the file. So even though locally I see tabs, locally I am hitting tab, the file has spaces? What a cluster man. Thanks for the help on that. So are we good with the web edits or should I just create new PRs for my fixes?

Contributor

BradChase2011 commented Mar 3, 2017

@rmarinho YUP!! That was it, something switched me over to Insert Spaces. UGHH!!! What is weird is I am hitting tab but that setting must switch them out automatically on the file. So even though locally I see tabs, locally I am hitting tab, the file has spaces? What a cluster man. Thanks for the help on that. So are we good with the web edits or should I just create new PRs for my fixes?

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 3, 2017

Member

You can just fix the changes locally and force push the branch

Member

rmarinho commented Mar 3, 2017

You can just fix the changes locally and force push the branch

@BradChase2011

This comment has been minimized.

Show comment
Hide comment
@BradChase2011

BradChase2011 Mar 3, 2017

Contributor

@rmarinho ok if you get a chance check the latest formatting, im checked in.

Contributor

BradChase2011 commented Mar 3, 2017

@rmarinho ok if you get a chance check the latest formatting, im checked in.

Brad Chase added some commits Mar 3, 2017

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 3, 2017

Member

Ok you are almost there @BradChase2011 :) still wp8 is wrong

Member

rmarinho commented Mar 3, 2017

Ok you are almost there @BradChase2011 :) still wp8 is wrong

@BradChase2011

This comment has been minimized.

Show comment
Hide comment
@BradChase2011

BradChase2011 Mar 4, 2017

Contributor

@rmarinho hah ok worst checkin ever, when is the awards ceremony? I forced it again, lets hope 8th times a charm! Good news is its fixed for now on, yayyy.

Contributor

BradChase2011 commented Mar 4, 2017

@rmarinho hah ok worst checkin ever, when is the awards ceremony? I forced it again, lets hope 8th times a charm! Good news is its fixed for now on, yayyy.

@rmarinho rmarinho merged commit 87f0bfa into xamarin:master Mar 9, 2017

@samhouts samhouts added D-15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts added this to the 2.3.5 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment