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

Feature: Improve Performance of MergeManyChangeSets with fewer changesets #829

Merged

Conversation

dwcullop
Copy link
Member

@dwcullop dwcullop commented Jan 8, 2024

Description

This PR improves the performance of the various MergeManyChangeSet operators by reducing the number of changeset that are emitted. Instead of one changeset per change in child changesets, the changes are held until they've all been processed, and the result is then emitted as a single changeset.

Impact

All changes from a parent changeset will result in a single downstream changeset. If it adds 2 items and removes 3 others, then the downstream will see a single changeset that adds of the child items from the added parent items and removes all of the child items from the 3 removed parent items.

Updates will result in a single changeset with removal of old child items/adding of new child items.

Logic Overview

By setting a flag before handling any changes to the parent changeset, we can detect if changes to the child changeset are due to parent changes (add/remove) we can avoid emitting them. At the end of handling a parent changeset, all of the changes can be emitted at once.

When the flag is not set, then the child changeset are not the result of parental changes, and they can be emitted immediately.

Other Changes

Creates an overload for SubscribeSafe extension method that enables it to be invoked with Action objects instead of just IObserver<T>.

Testing

  • Updated unit tests to expect fewer changesets as appropriate
  • Added a new test to confirm clearing the parent cache results in a single changeset removing all the children

@dwcullop dwcullop self-assigned this Jan 8, 2024
@dwcullop dwcullop enabled auto-merge (squash) January 8, 2024 19:20
Copy link
Collaborator

@RolandPheasant RolandPheasant left a comment

Choose a reason for hiding this comment

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

Smart optimisation

@dwcullop
Copy link
Member Author

dwcullop commented Jan 9, 2024

Smart optimisation

I had wanted this behavior before, but it wasn't obvious how... When I started digging into TransformMany, I realized someone (you, presumably) went to a lot of trouble to minimize the number of changesets and that I should really go back and figure it out so the behavior is consistent.

@dwcullop dwcullop disabled auto-merge January 9, 2024 20:27
@dwcullop dwcullop merged commit 7585ae0 into reactivemarbles:main Jan 9, 2024
1 check passed
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants