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

Improve performance of collection methods in entity adapters #440

Merged
merged 5 commits into from
Oct 3, 2017

Conversation

dinvlad
Copy link
Contributor

@dinvlad dinvlad commented Sep 29, 2017

Prior to this fix, collection methods in sorted_state_adapter
ran in quadratic time, because they iterated over the collection
with single-item mutators, each of which also running in linear time.

Now, we first collect all modifications (each in constant time),
then sort them in linearithmic time, and finally merge
them with the existing sorted array linearly.

As a result, we can improve performance of collection methods
to N + M log M, where N is the number of existing items
and M is the number of modifications.

Single-item methods are now expressed through those
for collections, still running linearly.


EDIT: Also updated methods for unsorted collections:

These methods ran in quadratic time by interating over
the collection and applying a linear algorithm for each item.

Now, we modify entity hashes first (each in constant time),
and then update the array of their ids in linear time.

As a result, all operations now occur linearly.

We also simplified logic for single-item operations
by expressing them through those for collections.

The removal methods also apply to sorted collections
(because they preserve their order).

Relevant discussion: #421

Prior to this fix, collection methods in sorted_state_adapter
ran in quadratic time, because they iterated over the collection
with single-item mutators, each of which also running in linear time.

Now, we first collect all modifications (each in constant time),
then sort them in linearithmic time, and finally merge
them with the existing sorted array linearly.

As a result, we can improve performance of collection methods
to N + M log M, where N is the number of existing items
and M is the number of modifications.

Single-item methods are now expressed through those
for collections, still running linearly.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.377% when pulling 46b62b9 on dinvlad:master into e63720e on ngrx:master.

…_adapter

Removing "holes" created by updateManyMutably()
should be there instead of merge().
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.397% when pulling 6d2999c on dinvlad:master into e63720e on ngrx:master.

It should return "true" unconditionally (probably worth a test).
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.404% when pulling ee9067c on dinvlad:master into e63720e on ngrx:master.

These methods ran in quadratic time by interating over
the collection and applying a linear algorithm for each item.

Now, we modify entity hashes first (each in constant time),
and then update the array of their ids in linear time.

As a result, all operations now occur linearly.

We also simplified logic for single-item operations
by expressing them through those for collections.

The removal methods also apply to sorted collections
(because they preserve their order).
@dinvlad dinvlad changed the title Improve performance for sorted entity adapter Improve performance of collection methods in entity adapters Sep 29, 2017
CircleCI failed the build because includes() is not supported
by its runtime yet. We use an alternative syntax to fix that.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 91.528% when pulling 8efba70 on dinvlad:master into e63720e on ngrx:master.

@MikeRyanDev MikeRyanDev merged commit 7fa0cf2 into ngrx:master Oct 3, 2017
@MikeRyanDev
Copy link
Member

Thanks @dinvlad!

@dinvlad
Copy link
Contributor Author

dinvlad commented Oct 4, 2017

Glad to help!

Something I haven't addressed is if an entity is updated with a new ID (as determined by selectId()), then the sorted code doesn't check if this ID already exists (in other words, another entity may already correspond to this ID). In that case, the code may put 2 identical IDs in the new sequence of state IDs. Additionally, they may actually violate the order, e.g. if the other entity is larger than a particular element, but the new entity is smaller. The new entity will then overwrite the other one, but its ID will appear twice in the sequence:

(new entity) ... < (element) ... > (new entity) <= ...

To prevent that, we could delete state.entities[selectId(updated)]; in addition to

delete state.entities[update.id];

However, this case prompts the question: should such updates be considered legal in the first place? After all, they modify multiple entities with a 'single' update and it may not be clear which update should take precedence (imagine doing multiple of these at the same time).

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