fix most memory leaks from opening and closing Dynamo Splash Screen #14344
Conversation
This file contains hidden or 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
mjkkirschner
commented
Aug 29, 2023
| Close(); | ||
| viewModel.RequestClose -= SplashScreenRequestClose; | ||
| //viewModel?.Model.ShutDown(false); | ||
| //TODO ensure viewmodel is shutdown by view being disposed. |
Member
Author
There was a problem hiding this comment.
I believe I verified this, but I will double check - IMO there is no reason to shutdown the view model because what we really need to do is dispose the DynamoView which will shutdown the view model - but again - I will verify.
Member
Author
There was a problem hiding this comment.
@QilongTang I've verified that the close call on DynamoView will shutdown the model and view model under normal circumstances. When in testmode this is handled by the teardown logic of most test base classes. I think this change is safe.
PTAL.
mjkkirschner
commented
Aug 29, 2023
|
|
9 tasks
QilongTang
approved these changes
Sep 1, 2023
mjkkirschner
added a commit
to mjkkirschner/Dynamo
that referenced
this pull request
Sep 1, 2023
…ynamoDS#14344) * fixing mem leaks in progress * fix test and remove todo
| DynamoViewModel.CurrentSpaceViewModel.Model.NodeAdded -= OnNodeAdded; | ||
| DynamoViewModel.Model.Logger.NotificationLogged -= OnNotificationLogged; | ||
| CurrentWorkspace.RequestPackageDependencies -= PythonDependencies.AddPythonPackageDependency; | ||
| if (LoadedParams != null) |
Contributor
There was a problem hiding this comment.
Were there null reference exceptions being thrown with all these objects earlier?
Member
Author
There was a problem hiding this comment.
yes during shutdown directly from the splash screen being closed.
mjkkirschner
added a commit
that referenced
this pull request
Sep 1, 2023
* fix most memory leaks from opening and closing Dynamo Splash Screen (#14344) * fixing mem leaks in progress * fix test and remove todo * attempt to fix dispose issue in docs browser view get rid of redudant try/catch cleanup log event in docs browser extension replace item produced lambda with named method fix broken dispose logic in nodeautocompletesearchcontrol - it was always hanging onto static event implement dispose on viewextensioncommandexec on dynamoviewmodel dispose, also dispose all workspaces - their remove methods are not called... fix entry added lambda in searchviewmodel dynamoview now disposes workspace view and node autocomplete search - TODO here. workspaceview implement idisposable and cleans up view model subscriptions direct manip extension was not cleaning up settings prop changed. graph meta data view was not cleaning up current workspace cleared event attempt to cleanup browser com refs... not sure this is helping * review comment
mjkkirschner
added a commit
that referenced
this pull request
Sep 11, 2023
* fix most memory leaks from opening and closing Dynamo Splash Screen (#14344) * fixing mem leaks in progress * fix test and remove todo * 2.19.1 disable network traffic WIP (#14393) * cherry pick analytics issue (#13328) * update * update * null checking * update * Update CoreNodeModelWpfResources.Designer.cs Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> * Revert "cherry pick analytics issue (#13328)" This reverts commit 6f9761d. * cherry pick analytics issue (#13328) (#14085) * update * update * null checking * update * Update CoreNodeModelWpfResources.Designer.cs Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> * Disable net traffic (#14083) * disable net traffic example * Update PackageManagerViewExtension.cs * update * Update DynamoModel.cs * Update PackageManagerViewExtension.cs * Update PackageManagerViewExtension.cs --------- Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> * Update PreferencesViewModel.cs: crash due to null feature flags (#14087) * Update PreferencesViewModel.cs * Update PackageManagerViewExtensionTests.cs --------- Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> * changes * remove comment * comments and disable notifications --------- Co-authored-by: pinzart90 <46732933+pinzart90@users.noreply.github.com> Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> --------- Co-authored-by: pinzart90 <46732933+pinzart90@users.noreply.github.com> Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com>
mjkkirschner
added a commit
that referenced
this pull request
Sep 19, 2023
* fix most memory leaks from opening and closing Dynamo Splash Screen (#14344) * fixing mem leaks in progress * fix test and remove todo * 2.19.1 disable network traffic WIP (#14393) * cherry pick analytics issue (#13328) * update * update * null checking * update * Update CoreNodeModelWpfResources.Designer.cs Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> * Revert "cherry pick analytics issue (#13328)" This reverts commit 6f9761d. * cherry pick analytics issue (#13328) (#14085) * update * update * null checking * update * Update CoreNodeModelWpfResources.Designer.cs Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> * Disable net traffic (#14083) * disable net traffic example * Update PackageManagerViewExtension.cs * update * Update DynamoModel.cs * Update PackageManagerViewExtension.cs * Update PackageManagerViewExtension.cs --------- Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> * Update PreferencesViewModel.cs: crash due to null feature flags (#14087) * Update PreferencesViewModel.cs * Update PackageManagerViewExtensionTests.cs --------- Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> * changes * remove comment * comments and disable notifications --------- Co-authored-by: pinzart90 <46732933+pinzart90@users.noreply.github.com> Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> * add python tests * move project * revert --------- Co-authored-by: pinzart90 <46732933+pinzart90@users.noreply.github.com> Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Purpose
Before this PR each time you opened Dynamo and closed the splash screen, > 10 megs would leak as well as an entire DynamoView and everything else in the tree.
This PR leaves only 1 extra DynamoView, I need to investigate the TODO before this is merged.
Will continue looking at more complex workflows when I have bandwidth.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Fixes some memory leaks from repeatedly opening and closing Dynamo's splash screen
Reviewers