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

Memory store putSync performance improvement #227

Merged
merged 1 commit into from
Jan 8, 2020

Conversation

msssk
Copy link
Contributor

@msssk msssk commented Jan 6, 2020

Fixes #183

The existing logic splices out a modified element and splices it back in
any time an update is made with putSync. With large arrays the performance
of Array#splice is bad. Additionally, the store's index is rebuilt to account
for moved items.

This PR adds a check to see if the item being put has moved. If not, the array
is modified in-place with no splicing, and the store's index is not rebuilt.

Some boundary tests for putSync are also added to the test suite.

Thanks to @nijuyr for the troubleshooting and approach.

The existing logic splices out a modified element and splices it back in
any time an update is made with putSync. With large arrays the performance
of Array#splice is bad. Additionally, the store's index is rebuilt to account
for moved items.

This PR adds a check to see if the item being put has moved. If not, the array
is modified in-place with no splicing, and the store's index is not rebuilt.

Some boundary tests for putSync are also added to the test suite.
@msssk msssk requested a review from dylans January 7, 2020 02:15
@msssk msssk merged commit dd6d193 into SitePen:master Jan 8, 2020
@nijuyr
Copy link

nijuyr commented Jan 17, 2020

I'm happy that the fix is in place.
However it is odd that not all suggested changes were included.
In The PR I made, not only the fix for the issue was included but it also cleared up things with some improvements in the overall code maintainability & readability.

  1. No arbitrary options check in functional lines of code by evaluating options first.
  2. Clear comments for where important logic is placed

I'd sure not be happy seeing code such as
L101

					if (eventType === 'update') {
						--destination;
					}

Again, thanks for having the fix in but I believe the next update can include some code improvement as well.

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.

Questionable PutSync performance
3 participants