-
Notifications
You must be signed in to change notification settings - Fork 357
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
Merge setup flow into a single project #148
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
I was doing some perf analysis of DevHome and I decided to do some measurements with this change. I can confirm we do see a great improvement with this change. Composite is what I am using as a baseline which is another change I had done to help improve perf. Setup Flow merge also has that change in the experiment I did.
|
It's great to see that merging dlls improves the performance 🔥🔥 Should we keep pages (connected components) in separate folders to maintain the original structure but with a single dll? I am thinking maybe having all Example:
|
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.
just noticed this is still here, we can delete this entire folder. tools/SetupFlow/DevHome.SetupFlow.DevVolume
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.
I considered it, but thought it may be better to follow the same structure as in the other projects by having the folders for viewmodels, views, services, etc. at the top level. We may lose a bit of being able to see "all of this is related to appmanagement", but I'd think it's easier to see the more global structure or the parallels between how each page works like this. I can change it if you want, though |
# Conflicts: # tools/SetupFlow/DevHome.SetupFlow/ViewModels/RepoConfigViewModel.cs # tools/SetupFlow/DevHome.SetupFlow/Views/AddRepoDialog.xaml # tools/SetupFlow/DevHome.SetupFlow/Views/LoadingView.xaml # tools/SetupFlow/DevHome.SetupFlow/Views/RepoConfigView.xaml
# Conflicts: # tools/SetupFlow/DevHome.SetupFlow/Controls/SetupShell.xaml
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Summary of the pull request
This merges all of the projects that make up the setup flow into a single project. This apparently gives us a major performance boost in loading all the pages in the flow
References and relevant issues
Detailed description of the pull request / Additional comments
Moved everything under the DevHome.SetupFlow, except the ComInterop and Elevated* Projects.
Adjusted the namespaces to remove the parts specific to each page. For example, DevHome.SetupFlow.MainPage.Views was merged into DevHome.SetupFlow.Views.
Besides adjusting the usings in all files, the only major change was in the ServiceExtensions.cs as that had a version on each project. I copied all the methods into a single file.
See before / after:
Validation steps performed
PR checklist