[Discussion] NGRX effects and selectors optimization

[jlipka]

Hello everyone,

I don't have a concrete Github issue to address this topic, but would like to mention two code parts which might be optimized in favor of small performance improvements (?). I'm not sure if they really make a difference in that way, so I would like to put the topics up for discussion. I'm struggling to measure the changes in a proper way.

I know that Stack Overflow was recommended as a discussion channel in the Lyrasis/DSpace wiki, but I think here is exactly the place where to find the right people who are interested in these topics. So, I would like to ask for your patience.

---

NGRX effect: replace "withLatestFrom" with "concatLatestFrom"

In the file / NGRX effect src/app/submission/objects/submission-objects.effects.ts, withLatestFrom is used in various places. If I insert a console.log in the appropriate place, you can see that the NGRX state is emitted hundreds to thousands of times.

Change original code (around L188 and other in same file) with:

withLatestFrom(this.store$.pipe(tap((foo) => console.log('state change triggered', foo))))

If you insert the console output in the original code, you will recognise that the complete* NGRX state is emitted very frequently (around 1500 console outputs with every page change, and many more within a Submission). However, I'm not sure whether this has an excessive impact on performance - the data is not processed any further, and the console.logs just make them visible... - but, in my opinion, it is clear that these are at least unnecessary emits and should therefore be suppressed - right?

*) We can choose a much more specific selector at these points later, depending on the use case, instead of retrieving the entire state! In this step, however, I would only like to deal with the operator in order to decide whether the change makes sense in principle and would like to know to what extent this has an effect.

Replace with:

concatLatestFrom(() => this.store$.pipe(tap((foo) => console.log('state change triggered', foo)))),

Consequently, the updated NGRX state is only retrieved subsequent to the execution of the corresponding actions.

Sources / Information

For example, when selecting data from the store with store.select, concatLatestFrom will prevent the selector from being evaluated until the source emits a value.
Source: NGRX operators

Use concatLatestFrom when you only need the state at the moment the action occurs. This can be more performant, especially if the state changes frequently.
NgRx Effects: A Comprehensive Guide

---

Caching of NGRX selectors

In the file / the NGRX submission selectors src/app/submission/selectors.ts various selectors are combined with each other. If I debug these parts and check the createSelector calls (e.g. with console.logs in the keySelector function), you can see that the same selectors are always (re-)created with the same parameters. Here I'm also not quite sure whether NGRX handles this in a performant way, or whether it would not be sufficient to create unique selectors once and then reuse them with a simple caching mechanism.

Original code (example, at least one other place in the file should be used with caching mechanism):

export function submissionObjectFromIdSelector(submissionId: string): MemoizedSelector<SubmissionState, SubmissionObjectEntry> {
  return keySelector<SubmissionState, SubmissionObjectEntry>(submissionSelector, 'objects', submissionId);
}

Modified code:

const cachedSubmissionObjectSelectors = new Map<string, MemoizedSelector<SubmissionState, SubmissionObjectEntry>>();

export function submissionObjectFromIdSelector(submissionId: string): MemoizedSelector<SubmissionState, SubmissionObjectEntry> {
  if (!cachedSubmissionObjectSelectors.has(submissionId)) {
    const selector = keySelector<SubmissionState, SubmissionObjectEntry>(submissionSelector, 'objects', submissionId);

    cachedSubmissionObjectSelectors.set(submissionId, selector);
  } else {
    savedCreations++;
  }

  return cachedSubmissionObjectSelectors.get(submissionId)!;
}

In purely quantitative terms, we can save lots of createSelector instances after a few click sequences using ‘MyDSpace’ and the submissions. Will you even notice these adjustments at runtime? I don't know exactly, and - as I said - I'm also not sure whether you achieve a big performance gain in terms of less memory consumption.

In the main branch (and an only slightly modified configuration) I have saved about 40 ‘createSelector’ calls after opening a single submission. While browsing further even more instances would have been created.
In our institutional repository (with many customisations and usage of entities etc) the caching saves about 130 createSelector calls when opening a single submission. Considering that my colleagues open many workspace and workflow items a day, I could imagine that this has an impact on browser performance - what do you think? Does the framework handle all these objects in a performant way, meaning my approach isn't as necessary as I expect?

At first glance, I couldn't find any negative aspects doing these changes, but this would have to be tested thoroughly in a pull request.

---

Do you think these are relevant and valuable changes?
Depending on the feedback, I might open a pull request(s) for these parts.

[ybnd]

Caching of NGRX selectors

I'd looked into this before but didn't see performance gains IIRC.
If you want to open a PR I'm open to reviewing it and measuring again though

Could you look into using something like Lodash memoize instead of the map set/get approach you have above? Would make it easier to read.