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

Allow to assign Node.Name only once to avoid on-change subscriptions overhead #344

Merged
merged 7 commits into from
Jan 16, 2025

Conversation

SergeiPavlov
Copy link
Collaborator

@SergeiPavlov SergeiPavlov commented Jan 16, 2025

Dumps show that Nodes' on-Name-Change subscriptions take > 11MB
image

But they are not used, because the Domain Model is immutable.
And the App does not use provided API to modify the built model.

In this PR I've added checking to ensure Node.Name is allowed to be assigned only once to non-null value.
It would be better to convert it into init setter, but will require massive refactoring.

Also:

  • Node doesn't implement IChangeNotifier anymore
  • Make Node.Name setter be internal

Copy link

@botinko botinko left a comment

Choose a reason for hiding this comment

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

Sorry, couldnt follow for all of PR changes. Approve just in case.

@SergeiPavlov SergeiPavlov merged commit 94c97f8 into master-servicetitan Jan 16, 2025
5 checks passed
@SergeiPavlov SergeiPavlov deleted the nodeName branch January 16, 2025 18:10
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.

3 participants