Skip to content

Commit

Permalink
perf(core): improve speed of inserting new items
Browse files Browse the repository at this point in the history
Related: #732
  • Loading branch information
B4nan committed Aug 11, 2020
1 parent eeae2be commit bfeb2e3
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions packages/core/src/unit-of-work/UnitOfWork.ts
Original file line number Diff line number Diff line change
Expand Up @@ -351,10 +351,10 @@ export class UnitOfWork {
* clean up persist/remove stack from previous persist/remove calls for this entity done before flushing
*/
private cleanUpStack(stack: AnyEntity[], entity: AnyEntity): void {
for (const index in stack) {
if (stack[index] === entity) {
stack.splice(+index, 1);
}
const idx = stack.indexOf(entity);

if (idx !== -1) {
stack.splice(idx, 1);

This comment has been minimized.

Copy link
@marcj

marcj Aug 11, 2020

Maybe stack: Set<AnyEntity>() yields better performance for your use-case. If it's a real 'stack' you should just call pop.

This comment has been minimized.

Copy link
@B4nan

B4nan Aug 11, 2020

Author Member

This is about cleaning up different stack than the one being processed. Like when you add to persist stack, this is used to remove the entity from remove stack, and vice versa.

This comment has been minimized.

Copy link
@marcj

marcj Aug 11, 2020

Yes, then you should definitely instead of arrays use Sets here for both lists, persist and remove. Using pure arrays hurt performance a lot when dealing with a lot of items, since complexity is O(n) (indexOf), Set has on the other side O(1).

This comment has been minimized.

Copy link
@B4nan

B4nan Aug 11, 2020

Author Member

Does Set maintain the order of items? I believe it was required, but maybe with recent changes it is no longer true, will need to try.

This comment has been minimized.

Copy link
@marcj

marcj Aug 11, 2020

Not ordered. But order should not be important for persist/remove tracking, should it?

This comment has been minimized.

Copy link
@B4nan

B4nan Aug 11, 2020

Author Member

Originally it was, but currently the order of entities is computed based on metadata (their connections), so it should not matter I guess.

This comment has been minimized.

Copy link
@merceyz

This comment has been minimized.

Copy link
@B4nan

B4nan Aug 11, 2020

Author Member

But for this we need a Map, as I need to look up based on either the object or its uuid, right?

This comment has been minimized.

Copy link
@marcj

marcj Aug 11, 2020

Why a map?

This comment has been minimized.

Copy link
@B4nan

B4nan Aug 11, 2020

Author Member

Because this method is about removing items from stack based in identity. When you persist, this makes sure the entity is not inside remove stack, and vice versa.

This comment has been minimized.

Copy link
@merceyz

merceyz Aug 11, 2020

Contributor

If you need to lookup based on both you'd need two maps.

This comment has been minimized.

Copy link
@marcj

marcj Aug 11, 2020

just call stack.delete(entity);

This comment has been minimized.

Copy link
@B4nan

B4nan Aug 11, 2020

Author Member

ah, perfect!

}
}

Expand Down

0 comments on commit bfeb2e3

Please sign in to comment.