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

Introduce Dynamo Service Mode as new CLI flag #13659

Merged
merged 20 commits into from
Feb 16, 2023
Merged

Introduce Dynamo Service Mode as new CLI flag #13659

merged 20 commits into from
Feb 16, 2023

Conversation

QilongTang
Copy link
Contributor

@QilongTang QilongTang commented Jan 5, 2023

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

This is a PR introducing service mode -s as CLI flag to minimize code startup time. I have finished going through most of the initialization logic and decided to skip certain ones with graph opening tested.

Here is the current list of steps that can be skipped:
DynamoModel:

  • Reading ExeConfiguration about DisableAnalytics flag for Alias Team
  • Analytics Client initialization
  • Dynamo settings migrator
  • Adding default trust locations
  • Python template loading
  • Search Model
  • LinterManager initialization
  • AuthenticationManager initialization
  • UpdateManager.CheckForProductUpdate Call (I assume this call is expensive that connecting with AWS bucket to get latest Dynamo Release info)
  • Package Manager extension loading

DynamoViewModel:

  • Incanvas Search ViewModel initialization
  • Node AutoComplete Search ViewModel initialization
  • Node display in the library

image

Performance gain:
Test env: Macbook M1 - MacOS - Parallel - Win 11 setup
The memory diff of launching DynamoWPF CLI in keep-alive mode (what player is using)
Before: ~116mb

After: ~104mb roughly 10% gain.
image

The profiling time (DotTrace - Trace Mode) diff of launching DynamoWPF CLI in keep-alive mode (what player is using)

Before: ~10300ms

After: ~7900ms with roughly ~24% gain in the current state of PR. I suggest Team Morpheus wrap up the current work and introduce the service mode officially.
image

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

Introduce Dynamo Service Mode for CLI usage and minimal cold start time.

Reviewers

@DynamoDS/dynamo

FYIs

@Amoursol

QilongTang and others added 6 commits January 6, 2023 22:39
* Removing CanToggleLoginState (#13657)

* Dyn 5488 add horizontal scroll bar (#13661)

* DYN-5488-Add-Horizontal-Scrollbar

When showing the documentation for packages the PackageDetailView extension was not showing the horizontal scroll bar then I added a change so that will be shown when the content is bigger than the usual width.

* DYN-5488-Add-Horizontal-Scrollbar

The HorizontalScrollBar will be shown only in the DataGrid due that if is shown in the complete SideBar is showing a weird white box.

* Disable loading extensions and view extensions in service mode.

Co-authored-by: filipeotero <[email protected]>
Co-authored-by: Roberto T <[email protected]>
@QilongTang QilongTang marked this pull request as ready for review January 13, 2023 17:15
@QilongTang QilongTang changed the title [DRAFT] Service Mode Draft PR for discussion Introduce Dynamo Service Mode as new CLI flag Jan 13, 2023
@QilongTang
Copy link
Contributor Author

I guess I could add one CLI test for this, but I am uncertain if this PR is meant for merge at all. Theoretically, we could just deploy this branch build.

/// <param name="info">Host analytics info specifying Dynamo launching host related information.</param>
/// <param name="isServiceMode">Boolean indication of launching Dynamo in service mode, this mode is optimized for minimal launch time.</param>
/// <returns></returns>
public static DynamoModel MakeCLIModel(string asmPath, string userDataFolder, string commonDataFolder, HostAnalyticsInfo info = new HostAnalyticsInfo(), bool isServiceMode = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have also added a couple of flags last year :). I guess we need to think of something soon to better consolidate all the MakeModel constructs. Perhaps a startup config file where you can enable/disable features/extensions etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, currently the code (constructors) used for CLI are a bit messy


NodeFactory = new NodeFactory();
NodeFactory.MessageLogged += LogMessage;

//Initialize the ExtensionManager with the CommonDataDirectory so that extensions found here are checked first for dll's with signed certificates
extensionManager = new ExtensionManager(new[] { PathManager.CommonDataDirectory });
extensionManager.MessageLogged += LogMessage;
var extensions = config.Extensions ?? LoadExtensions();
var extensions = config.Extensions ?? new List<IExtension>();
if (!extensions.Any() && !IsServiceMode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a list somewhere with all our default extensions and what are they doing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In code, we dont maintain a list. We only maintain the list using xml manifest files in extensions and viewextensions folder

@@ -84,11 +84,13 @@ public override void Loaded(ViewLoadedParams viewLoadedParams)
// as we may have installed a missing package.

DependencyView.CustomNodeManager = (CustomNodeManager)viewLoadedParams.StartupParams.CustomNodeManager;

pmExtension.PackageLoader.PackgeLoaded += (package) =>
if (pmExtension != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

If pmExtenions does not exist does it makes sense to load this one or we can simply return here at the beginning ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are other types of workspace dependencies (e.g. local dependency, custom node workspace dependency) that are not depending on pmExtension, so it still make sense to load I believe.

@QilongTang
Copy link
Contributor Author

@mjkkirschner @BogdanZavu @reddyashish @zeusongit @pinzart @aparajit-pratap @sm6srw All tests are passing so if you would like to make this service mode official for CLI, we can do that as well now.

# Conflicts:
#	src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs
@saintentropy saintentropy mentioned this pull request Feb 9, 2023
8 tasks
* Add ServiceMode to Linux

* Load extensions in service mode

* Do not load preference setting file in ServiceMode

---------

Co-authored-by: Craig Long <[email protected]>
@QilongTang
Copy link
Contributor Author

Both tests reported passing locally. Merging

@QilongTang QilongTang merged commit e742707 into master Feb 16, 2023
@QilongTang QilongTang deleted the ServiceMode branch February 16, 2023 15:08
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.

5 participants