fix selectorGetter performance issue (facebookexperimental#1515)

Summary:
The current algorithm for updating selector dependencies in the data flow graph is expensive.  Currently we update the graph every time a new dependency is added, which scales very poorly.  This optimization only updates the graph with all sycnhronous dependencies after the synchronous selector evaluation completes.

We still have to update the graph after every asynchronous dependency is added since we need to know if we have to re-evaluate the selector if one of them is updated before the selector completely resolves.

This optimization significantly helps the common case for synchronous dependencies:
```
Improvement  Number of synchronous dependencies
2x           100
4x           1,000
40x          10,000
```

facebookexperimental#914 Please refer this issue for detail

Pull Request resolved: facebookexperimental#1515

Test Plan: Unit tests from D34100807 check for proper re-evaluation of updated async dependencies before async selectors resolve.

Differential Revision: https://www.internalfb.com/diff/D33825247?entry_point=27

Pulled By: drarmstr

fbshipit-source-id: 6df53e7bca2a2284c350149c662d7a7e7e31ca52

CHANGELOG.md

@@ -4,6 +4,7 @@
 **_Add new changes here as they land_**
 
 - The `default` value is now optional for `atom()` and `atomFamily()`.  If not provided the atom will initialize to a pending state. (#1639)
+- Significant optimization for selector evaluations.  2x improvement with 100 dependencies, 4x with 1,000, and 40x with 10,000. (#1515, #914)
 - `shouldNotBeFrozen` now works in JS environment without `Window` interface. (#1571)
 - Avoid spurious console errors from effects when calling `setSelf()` from `onSet()` handlers. (#1589)
 - Better error reporting when selectors provide inconsistent results (#1696)
@@ -35,6 +36,7 @@
 - Add `.isRetained()` method for Snapshots and check if snapshot is already released when using `.retain()` (#1546)
 
 ### Other Fixes and Optimizations
+
 - Reduce overhead of snapshot cloning
   - Only clone the current snapshot for callbacks if the callback actually uses it. (#1501)
   - Cache the cloned snapshots from callbacks unless there was a state change. (#1533)

packages/recoil/core/__tests__/Recoil_perf-test.js

@@ -13,6 +13,7 @@
 import type {Loadable, RecoilState, RecoilValue} from '../../Recoil_index';
 
 const {atom, selector} = require('../../Recoil_index');
+const {waitForAll} = require('../../recoil_values/Recoil_WaitFor');
 const {
   getRecoilValueAsLoadable,
   setRecoilValue,
@@ -26,11 +27,15 @@ const ITERATIONS = [1]; // Avoid iterating for automated testing
 // const ITERATIONS = [10, 100, 1000, 10000];
 // const ITERATIONS = [10, 100, 1000, 10000, 100000];
 
-function testPerf(name: string, fn: number => void) {
+function testPerf(
+  name: string,
+  fn: ({iterations: number, begin: () => void}) => void,
+) {
   test.each(ITERATIONS)(name, iterations => {
     store = makeStore();
-    const BEGIN = performance.now();
-    fn(iterations);
+    let BEGIN = performance.now();
+    const begin = () => void (BEGIN = performance.now());
+    fn({iterations, begin});
     const END = performance.now();
     console.log(`${name}(${iterations})`, END - BEGIN);
   });
@@ -80,27 +85,30 @@ const helpersSelector = () =>
   });
 const getHelpers = () => getNodeValue(helpersSelector());
 
-testPerf('creating n atoms', iterations => {
+testPerf('creating n atoms', ({iterations}) => {
   createAtoms(iterations);
 });
 
-testPerf('getting n atoms', iterations => {
+testPerf('getting n atoms', ({iterations, begin}) => {
+  begin();
   const atoms = createAtoms(iterations);
   for (const node of atoms) {
     getNodeValue(node);
   }
 });
 
-testPerf('setting n atoms', iterations => {
+testPerf('setting n atoms', ({iterations, begin}) => {
   const atoms = createAtoms(iterations);
+  begin();
   for (const node of atoms) {
     setNode(node, 'SET');
   }
 });
 
-testPerf('cloning n snapshots', iterations => {
+testPerf('cloning n snapshots', ({iterations, begin}) => {
   const atoms = createAtoms(iterations);
   const {getSnapshot} = getHelpers();
+  begin();
   for (const node of atoms) {
     // Set node to avoid hitting cached snapshots
     setNode(node, 'SET');
@@ -109,3 +117,9 @@ testPerf('cloning n snapshots', iterations => {
     expect(snapshot.getLoadable(node).contents).toBe('SET');
   }
 });
+
+testPerf('Selector dependencies', ({iterations, begin}) => {
+  const atoms = createAtoms(iterations);
+  begin();
+  getNodeValue(waitForAll(atoms));
+});

packages/recoil/recoil_values/Recoil_selector.js

@@ -277,7 +277,7 @@ function selector<T>(
     depValues: DepValues,
   ): void {
     setCache(state, loadable, depValues);
-    setDepsInStore(store, state, new Set(depValues.keys()), executionId);
+    // setDepsInStore(store, state, new Set(depValues.keys()), executionId);
     notifyStoresOfResolvedAsync(store, executionId);
   }
 
@@ -655,10 +655,11 @@ function selector<T>(
     executionId: ExecutionId,
   ): [Loadable<T>, DepValues] {
     const endPerfBlock = startPerfBlock(key); // TODO T63965866: use execution ID here
-    let gateCallback = true;
+    let duringSynchronousExecution = true;
+    let duringAsynchronousExecution = true;
     const finishEvaluation = () => {
       endPerfBlock();
-      gateCallback = false;
+      duringAsynchronousExecution = false;
     };
 
     let result;
@@ -669,8 +670,6 @@ function selector<T>(
       loadingDepPromise: null,
     };
 
-    const depValues = new Map();
-
     /**
      * Starting a fresh set of deps that we'll be using to update state. We're
      * starting a new set versus adding it in existing state deps because
@@ -681,14 +680,16 @@ function selector<T>(
      * execution means the deps we discover below are our best guess at the
      * deps for the current/latest state in the store)
      */
+    const depValues = new Map();
     const deps = new Set();
-    setDepsInStore(store, state, deps, executionId);
 
     function getRecoilValue<S>(dep: RecoilValue<S>): S {
       const {key: depKey} = dep;
 
       deps.add(depKey);
-      setDepsInStore(store, state, deps, executionId);
+      if (!duringSynchronousExecution) {
+        setDepsInStore(store, state, deps, executionId);
+      }
 
       const depLoadable = getCachedNodeLoadable(store, state, depKey);
 
@@ -711,7 +712,7 @@ function selector<T>(
       fn: (SelectorCallbackInterface<T>) => (...Args) => Return,
     ): ((...Args) => Return) => {
       return (...args) => {
-        if (gateCallback) {
+        if (duringAsynchronousExecution) {
           throw err(
             'Callbacks from getCallback() should only be called asynchronously after the selector is evalutated.  It can be used for selectors to return objects with callbacks that can work with Recoil state without a subscription.',
           );
@@ -768,6 +769,8 @@ function selector<T>(
       loadable = loadableWithValue<T>(result);
     }
 
+    duringSynchronousExecution = false;
+    setDepsInStore(store, state, deps, executionId);
     return [loadable, depValues];
   }
 

packages/recoil/recoil_values/__tests__/Recoil_selector-test.js

@@ -29,6 +29,7 @@ let store,
   asyncSelector,
   resolvingAsyncSelector,
   stringAtom,
+  loadingAsyncSelector,
   flushPromisesAndTimers,
   DefaultValue,
   freshSnapshot;
@@ -52,6 +53,7 @@ const testRecoil = getRecoilTestFn(() => {
     asyncSelector,
     resolvingAsyncSelector,
     stringAtom,
+    loadingAsyncSelector,
     flushPromisesAndTimers,
   } = require('recoil-shared/__test_utils__/Recoil_TestingUtils'));
   ({noWait} = require('../Recoil_WaitFor'));
@@ -502,6 +504,81 @@ describe('Dependencies', () => {
       expect(valLoadable.valueMaybe()).toBe(10);
     },
   );
+
+  testRecoil('Dynamic deps discovered after await', async () => {
+    const testSnapshot = freshSnapshot();
+    testSnapshot.retain();
+
+    const myAtom = atom<number>({
+      key: 'selector dynamic dep after await atom',
+      default: 0,
+    });
+
+    let selectorRunCount = 0;
+    let selectorRunCompleteCount = 0;
+    const mySelector = selector({
+      key: 'selector dynamic dep after await selector',
+      get: async ({get}) => {
+        await Promise.resolve();
+        get(myAtom);
+        selectorRunCount++;
+        await new Promise(() => {});
+        selectorRunCompleteCount++;
+      },
+    });
+
+    testSnapshot.getLoadable(mySelector);
+    expect(testSnapshot.getLoadable(mySelector).state).toBe('loading');
+    await flushPromisesAndTimers();
+    expect(selectorRunCount).toBe(1);
+    expect(selectorRunCompleteCount).toBe(0);
+
+    const mappedSnapshot = testSnapshot.map(({set}) =>
+      set(myAtom, prev => prev + 1),
+    );
+    expect(mappedSnapshot.getLoadable(mySelector).state).toBe('loading');
+    await flushPromisesAndTimers();
+    expect(selectorRunCount).toBe(2);
+    expect(selectorRunCompleteCount).toBe(0);
+  });
+
+  testRecoil('Dynamic deps discovered after pending', async () => {
+    const pendingSelector = loadingAsyncSelector();
+    const testSnapshot = freshSnapshot();
+    testSnapshot.retain();
+
+    const myAtom = atom<number>({
+      key: 'selector dynamic dep after pending atom',
+      default: 0,
+    });
+
+    let selectorRunCount = 0;
+    let selectorRunCompleteCount = 0;
+    const mySelector = selector({
+      key: 'selector dynamic dep after pending selector',
+      get: async ({get}) => {
+        await Promise.resolve();
+        get(myAtom);
+        selectorRunCount++;
+        get(pendingSelector);
+        selectorRunCompleteCount++;
+      },
+    });
+
+    testSnapshot.getLoadable(mySelector);
+    expect(testSnapshot.getLoadable(mySelector).state).toBe('loading');
+    await flushPromisesAndTimers();
+    expect(selectorRunCount).toBe(1);
+    expect(selectorRunCompleteCount).toBe(0);
+
+    const mappedSnapshot = testSnapshot.map(({set}) =>
+      set(myAtom, prev => prev + 1),
+    );
+    expect(mappedSnapshot.getLoadable(mySelector).state).toBe('loading');
+    await flushPromisesAndTimers();
+    expect(selectorRunCount).toBe(2);
+    expect(selectorRunCompleteCount).toBe(0);
+  });
 });
 
 testRecoil('async set not supported', async () => {