RFC: Instrumented Selector

[blove]

Information

Summary

NgRx's createSelector and createSelectorFactory() functions currently do not support developer instrumentation for debugging the performance of a selector. the goal of this RFC is to propose the creation of an ergonomic API for instrumenting memorized selectors with no breaking changes that further has zero impact on the current performance of memorized selectors.

Background Information

As a user of NgRx, I have a selector that is either/both:

1. Being invoked more often than expected, or
2. Taking more CPU time to compute than expected.

As a user of NgRx, I would like to instrument a selector in a way that enables me to either/both:

1. Determine the cause (diff) for a selector to be recomputed, or
2. Determine the timing of computing a selector (potentially using the Performance API).

Currently, the createSelector() function accepts an array of inputs, invoking the the createSelectorFactor() function providing reference to the defaultMemoize function. This encapsulates the projectorFn function preventing the instrumentation of an existing MemoizedSelector.

Proposal

This RFC proposes a new createInstrumentedSelector() function to enable NgRx developers to instrument an existing MemoizedSelector instance.

type InstrumentedSelectorCallbackFn<T = any, V = any> = (
  previousValue: V,
  currentValue: V,
  projector: DefaultProjectorFn<T>,
  input: any[]
) => void;

createInstrumentedSelector<T = any, V = any>(
  selector: MemoizedSelector<T, V>,
  callback: InstrumentedSelectorCallbackFn<T, V>
);

Proposed implementation:

const selectSum = createSelector(selectState, (state) => state.sum);

const instrumentedSelectSum = createInstrumentSelector(
  selectSum,
  (previousValue, currentValue, projector, input) => {
    performance.mark('startSelectSum');
    const result = projector.apply(null, input);
    performance.mark('endSelectSum');
    performance.measure('selectSum');
    console.log(
      previousValue,
      currentValue,
      performance.getEntriesByName('selectSum').pop()
    );
  }
);

Describe any alternatives/workarounds you're currently using

Alternative Considered

An alternative solution considered requires no changes to the existing NgRx API. However, this alternative requires the developer to "re-create" an existing selector for instrumentation.

export const selectSum = createSelector(selectState, (state) => state.sum);

type InstrumentedSelectorCallbackFn<T = any, V = any> = (
  previousValue: V,
  currentValue: V,
  projectorFn: DefaultProjectorFn<T>,
  input: any[]
) => void;

function createInstrumentSelector<T = any, V = any>(
  ...input: [...any, InstrumentedSelectorCallbackFn<T, V>]
) {
  const callback = input.pop() as InstrumentedSelectorCallbackFn<T, V>;
  return createSelectorFactory<T, V>((projectorFn) =>
    defaultMemoize(projectorFn, undefined, (previousValue, currentValue) => {
      callback(previousValue, currentValue, projectorFn, input);
      return previousValue === currentValue;
    })
  )(input);
}

export const instrumentedSelectSum = createInstrumentSelector(
  selectState,
  (state) => state.sum,
  (previousValue, currentValue, projector, input) => {
    performance.mark('startSelectSum');
    const result = projector.apply(null, input);
    performance.mark('endSelectSum');
    performance.measure('selectSum');
    console.log(
      previousValue,
      currentValue,
      performance.getEntriesByName('selectSum').pop()
    );
  }
);

I would be willing to submit a PR to fix this issue

• [X] Yes
• [ ] No

[brandonroberts]

Thanks Brian for the interesting use case. I'm of the opinion that instead of adding another way to create a selector, we should allow the instrumentation of an existing selector if possible. That way we could have some sort of instrumentSelector function that's generic enough to be customized.

Thoughts?

[blove]

@brandonroberts agreed; that was the proposed createInstrumentSelector() function that would accept a MemoizedSelector instance and instrument with the provided callback function. Any other thoughts on the proposal? What are next steps?

[brandonroberts]

Oh ok, I see. There would need to be some tweaking on the types, tests, and docs to move it forward. My other question is an instrumented selector throwaway code after the performance analysis is done?

Also would like some thoughts from @timdeschryver @markostanimirovic

[timdeschryver]

First, I like the idea.

My initial thought is that the developer needs to first suspect that there's an issue, and that it's caused by a selector. What if we make this the default behavior while running in non-prod mode. For example, we could warn if a selector takes more than X ms and/or if it's running excessively.

If we don't want that, we could also opt for adding a config option to a selector? This could further be used to tweak its behavior, similar to how re-select does this. IIRC having more control over a selector is a feature that was asked before.

const customizedSelector = createSelector(
  state => state.a,
  state => state.b,
  (a, b) => a + b,
  {
    // New in 4.1: Pass options through to the built-in `defaultMemoize` function
    memoizeOptions: {
      equalityCheck: (a, b) => a === b,
      maxSize: 10,
      resultEqualityCheck: shallowEqual
    }
  }
)

For some inspiration, there's also reselect-debugger.

[blove]

I would generally be in favor of:

• Warnings when a selector's projection function computation exceeds X ms, provided there was a boolean flag to disable the warnings.
• Warnings when a selector is invoked X times in Y interval, again, provided there was a boolean flag to disable the warnings.
• Further enabling the instrumentation of a selector as necessary.

Would adding a configuration option to the createSelector() function be preferred over a new function, such as createInstrumentedSelector()?

[MikeRyanDev]

@timdeschryver One challenge to overcome with built-in performance monitoring for selectors is that we don't have a name or symbol to point developers to when a selector has bad performance. someSelector.name yields "memoized". Any thoughts on how we might overcome this?

[MikeRyanDev]

Implementation decisions in the algorithms of projector functions have stood out as a unique source of performance issues in NgRx apps that I've helped review. What I'd like to see achieved with this RFC is an API that lets me monitor the performance characteristics of a single selector function that I believe to be performing slowly. Additionally, I'd like this API o understand the selector graph so that it can help me understand where in the graph performance problems are occuring.

For example, let's say I have a tree of selectors:

   (A)   (B)   (C) 
    |     |     |
    ------    --
       |      |
      (D)    (E)
       |      |
        ------
          |
         (F)

If I want to monitor the performance of selector (F), I'd want this API to also monitor the performance of all parent selectors in the graph (A-E).

[timdeschryver]

@MikeRyanDev to be honest, haven't thought that far 😅 I think you're right that it would be hard (impossible?) to retrieve the name of the selector. The closest we can get here would be to monitor store.select instead of selectors, here we can find the callee. But this would make it harder to monitor the performance of a selector, and the selector tree.

Additionally, I'd like this API o understand the selector graph so that it can help me understand where in the graph performance problems are occuring.

This would make it powerful, gets a 👍 from me

[brandonroberts]

If we add some type of configuration object, we could potentially use that to identify the selector, and provide a default for existing selectors. It would also allow some flexibility in overriding the default projector, and memoization without having to use createSelectorFactory.

[alex-okrushko]

createSelectorFactory is a way to extend or adjust the behavior of selectors. Instead of increasing the API surface of selectors I'd suggest to use this function instead.

e.g. for measuring selector's performance one can create the following (one time implementation):

function instrumentedMemoize(
  marker: string,
  projectionFn: Function
): MemoizedProjection {
  function newProjectionFn() {
    console.log(marker);
    performance.mark(marker);
    const result = projectionFn.apply(null, arguments);
    console.log(
      `${marker} completed`,
      performance.measure(`${marker} completed`, marker).duration
    );
    return result;
  }

  return defaultMemoize(newProjectionFn);
}

export const createInstrumentedSelector: (
  marker: string
) => typeof createSelector = (marker: string) => {
  return (...input: unknown[]) => {
    return createSelectorFactory((projectionFn) =>
      instrumentedMemoize(marker, projectionFn)
    )(...input);
  };
};

now any selector can be instrumented in the app:

// BEFORE
export const getValue = createSelector(
  selectMyState,
  (myState) => {
    return myState.value;
  }
);

// AFTER
export const getValue = createInstrumentedSelector('my marker')(
  selectMyState,
  (myState) => {
    return myState.value;
  }
);

This approach allows teams to implement the instrumentation logic without expanding the API surface.

working example: https://stackblitz.com/edit/angular-kaf8rh?file=src/app/state.ts

[MikeRyanDev]

@alex-okrushko This is the solution Brian and I use with clients right now. There are several drawbacks to this approach:

1. Documentation on how to use createSelectorFactory is lacking
2. Using createSelectorFactory in this way is non-intuitive
3. This approach doesn't trace input selectors as this RFC suggests

It's point #3 that I think best demonstrates the need to bring this into the library. NgRx is viewed as the "enterprise" state management solution for Angular apps but has gaps in its API surface for developers to instrument and debug NgRx code. Diagnosing poor performance in NgRx code has been pervasive for Brian and I's consultancy. We both believe there should be better tooling and documentation in NgRx proper to help developers understand these performance issues independently without hiring an NgRx pro who knows how to leverage createSelectorFactory.