Removed unnecessary sort in sorted_state_adapter.ts#merge()

[jaidetree]

Hi Redux Toolkit maintainers,

While perusing the code, discovered the following:

redux-toolkit/src/entities/sorted_state_adapter.ts

Line 120 in 61ec24c

models.sort(sort)

Forked the repo, removed the sort, ran the tests with npm test and found no issues.

Since allEntities are sorted just after merging into state, seems like it would improve performance without the pre-sort.

Is a reason for the pre-sort I'm not seeing, that the tests are not capturing? If so I would really like to know why.

Thanks for your time!

[netlify]

Deploy preview for redux-starter-kit-docs ready!

Built with commit f65238c

https://deploy-preview-960--redux-starter-kit-docs.netlify.app

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f65238c:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration

[markerikson]

Hmm. So that line of code probably came over as part of the original port I did from @ngrx/entity. Would be good to compare vs their current logic in that function.

At this point in the code, models represents any new/added items that are being worked on. I'm not 100% sure that those need to be inserted in sorted order, but I'm also not sure that skipping sorting them will avoid changing any current behavior.

[jaidetree]

https://github.com/ngrx/platform/blob/0bb49409413b784b1cc55c2b59e67ed760041aa5/modules/entity/src/sorted_state_adapter.ts#L193-L214

function merge(models: any[], state: any): void {
    models.sort(sort);

    const ids: any[] = [];

    let i = 0;
    let j = 0;

    while (i < models.length && j < state.ids.length) {
      const model = models[i];
      const modelId = selectIdValue(model, selectId);
      const entityId = state.ids[j];
      const entity = state.entities[entityId];

      if (sort(model, entity) <= 0) {
        ids.push(modelId);
        i++;
      } else {
        ids.push(entityId);
        j++;
      }
    }

Looking into the original implementation, the use of pre-sort makes sense there. The pre-sorted list of models are used to conditionally append models[i].id or original state.entities[j].id depending on what the comparer sort function returns. In the case of redux toolkit, the allEntities array is sorted after merging with the .sort(sort) method. Given this added context, it seems like the pre-sort is not needed anymore right?

[markerikson]

Yeah, seems that way.

I do sorta wonder how much perf impact there is from us just calling .sort() every time here, but eh, I think we can probably skip worrying about that for now :)

Thanks for looking into this!

[jaidetree]

Thanks for your support. Since there's some personal guilt due to it being such a minor contribution, I setup a quick benchmark test:

https://jsbench.me/wkkmvg41xd/1

EDIT:
Realized the ngrx/entity speedup didn't update the state.entities like the array.sort example did. Now they're behaviorally equivalent.

Results so far:

OS	Browser	array.sort speedup	ngrx/entity speedup
OS X 11.2.3	Brave 1.21.77	~40%	-
OS X 11.2.3	Safari 14	-	~40%