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

Merge setup flow into a single project #148

Merged
merged 7 commits into from
Apr 6, 2023
Merged

Merge setup flow into a single project #148

merged 7 commits into from
Apr 6, 2023

Conversation

florelis
Copy link
Member

@florelis florelis commented Apr 4, 2023

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:

DevHome_URBmsJJYeu

DevHome_DnF6DJVmqs

Validation steps performed

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated

@florelis
Copy link
Member Author

florelis commented Apr 4, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@manodasanW
Copy link
Member

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.

  Composite (ms) Setup Flow merge (ms) Diff (ms)
Application Startup 14570 1120 13450
DashboardView Page Load 2740 332.09 2407.91
WhatsNew Page Load 1770 45.31 1724.69
Last Frame load at Startup 15870 2480 13390
SetupFlow Page Load 3640 110.07 3529.93

@AmelBawa-msft
Copy link
Contributor

AmelBawa-msft commented Apr 5, 2023

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 viewmodels/views/services from diff pages merged together makes the structure a little harder to analyze without context (?)

Example:

tools/
  SetupFlow/
    AppManagement/
    DevDrive/
    etc...

Copy link
Contributor

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

Copy link
Member Author

@florelis florelis Apr 5, 2023

Choose a reason for hiding this comment

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

Weird; I thought I had already deleted it all. Will delete

Edit: I knew I had already deleted it all!
image

@florelis
Copy link
Member Author

florelis commented Apr 5, 2023

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 viewmodels/views/services from diff pages merged together makes the structure a little harder to analyze without context (?)

Example:

tools/
  SetupFlow/
    AppManagement/
    DevDrive/
    etc...

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

florelis added 2 commits April 5, 2023 17:12
# 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
@florelis
Copy link
Member Author

florelis commented Apr 6, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@florelis florelis merged commit ebed362 into microsoft:main Apr 6, 2023
@florelis florelis deleted the mergeProjects branch April 6, 2023 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants