Earlier selector cache lookup

CHANGELOG.md

@@ -7,6 +7,8 @@
 - Update typing for family parameters to better support Map, Set, and classes with `toJSON()`. (#1709, #1703)
 - Cleanup potential memory leak when using async selectors (#1714)
 - Fix potentially hung async selectors when shared across multiple roots that depend on atoms initialized with promises that don't resolve (#1714)
+- Atom effects can initialize or set atoms to wrapped values (#1681)
+- Selector cache lookup optimization (#)
 
 ## 0.7 (2022-03-31)
 

packages/recoil/core/Recoil_Snapshot.js

@@ -83,7 +83,8 @@ class Snapshot {
       storeID: getNextStoreID(),
       getState: () => storeState,
       replaceState: replacer => {
-        storeState.currentTree = replacer(storeState.currentTree); // no batching so nextTree is never active
+        // no batching, so nextTree is never active
+        storeState.currentTree = replacer(storeState.currentTree);
       },
       getGraph: version => {
         const graphs = storeState.graphsByVersion;
@@ -281,18 +282,20 @@ function cloneStoreState(
   const storeState = store.getState();
   const version = bumpVersion ? getNextTreeStateVersion() : treeState.version;
   return {
-    currentTree: bumpVersion
-      ? {
-          // TODO snapshots shouldn't really have versions because a new version number
-          // is always assigned when the snapshot is gone to.
-          version,
-          stateID: version,
-          transactionMetadata: {...treeState.transactionMetadata},
-          dirtyAtoms: new Set(treeState.dirtyAtoms),
-          atomValues: treeState.atomValues.clone(),
-          nonvalidatedAtoms: treeState.nonvalidatedAtoms.clone(),
-        }
-      : treeState,
+    // Always clone the TreeState to isolate stores from accidental mutations.
+    // For example, reading a selector from a cloned snapshot shouldn't cache
+    // in the original treestate which may cause the original to skip
+    // initialization of upstream atoms.
+    currentTree: {
+      // TODO snapshots shouldn't really have versions because a new version number
+      // is always assigned when the snapshot is gone to.
+      version: bumpVersion ? version : treeState.version,
+      stateID: bumpVersion ? version : treeState.stateID,
+      transactionMetadata: {...treeState.transactionMetadata},
+      dirtyAtoms: new Set(treeState.dirtyAtoms),
+      atomValues: treeState.atomValues.clone(),
+      nonvalidatedAtoms: treeState.nonvalidatedAtoms.clone(),
+    },
     commitDepth: 0,
     nextTree: null,
     previousTree: null,

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

@@ -12,7 +12,7 @@
 
 import type {Loadable, RecoilState, RecoilValue} from '../../Recoil_index';
 
-const {atom, selector} = require('../../Recoil_index');
+const {atom, selector, selectorFamily} = require('../../Recoil_index');
 const {waitForAll} = require('../../recoil_values/Recoil_WaitFor');
 const {
   getRecoilValueAsLoadable,
@@ -22,22 +22,25 @@ const {performance} = require('perf_hooks');
 const {makeStore} = require('recoil-shared/__test_utils__/Recoil_TestingUtils');
 
 const ITERATIONS = [1]; // Avoid iterating for automated testing
-// const ITERATIONS = [2, 2];
+// const ITERATIONS = [100];
+// const ITERATIONS = [1000];
 // const ITERATIONS = [10, 100, 1000];
 // const ITERATIONS = [10, 100, 1000, 10000];
 // const ITERATIONS = [10, 100, 1000, 10000, 100000];
 
 function testPerf(
   name: string,
-  fn: ({iterations: number, begin: () => void}) => void,
+  fn: ({iterations: number, perf: (() => void) => void}) => void,
 ) {
   test.each(ITERATIONS)(name, iterations => {
     store = makeStore();
-    let BEGIN = performance.now();
-    const begin = () => void (BEGIN = performance.now());
-    fn({iterations, begin});
-    const END = performance.now();
-    console.log(`${name}(${iterations})`, END - BEGIN);
+    const perf = cb => {
+      const BEGIN = performance.now();
+      cb();
+      const END = performance.now();
+      console.log(`${name}(${iterations})`, END - BEGIN);
+    };
+    fn({iterations, perf});
   });
 }
 
@@ -85,41 +88,106 @@ const helpersSelector = () =>
   });
 const getHelpers = () => getNodeValue(helpersSelector());
 
-testPerf('creating n atoms', ({iterations}) => {
+testPerf('create n atoms', ({iterations}) => {
   createAtoms(iterations);
 });
 
-testPerf('getting n atoms', ({iterations, begin}) => {
-  begin();
+testPerf('get n atoms', ({iterations, perf}) => {
   const atoms = createAtoms(iterations);
-  for (const node of atoms) {
-    getNodeValue(node);
-  }
+  perf(() => {
+    for (const node of atoms) {
+      getNodeValue(node);
+    }
+  });
 });
 
-testPerf('setting n atoms', ({iterations, begin}) => {
+testPerf('set n atoms', ({iterations, perf}) => {
   const atoms = createAtoms(iterations);
-  begin();
-  for (const node of atoms) {
-    setNode(node, 'SET');
-  }
+  perf(() => {
+    for (const node of atoms) {
+      setNode(node, 'SET');
+    }
+  });
+});
+
+testPerf('get n selectors', ({iterations, perf}) => {
+  const atoms = createAtoms(iterations);
+  const testFamily = selectorFamily({
+    key: 'PERF-getselectors',
+    get:
+      id =>
+      ({get}) =>
+        get(atoms[id]) + get(atoms[0]),
+  });
+  perf(() => {
+    for (let i = 0; i < iterations; i++) {
+      getNodeValue(testFamily(i));
+    }
+  });
 });
 
-testPerf('cloning n snapshots', ({iterations, begin}) => {
+testPerf('clone n snapshots', ({iterations, perf}) => {
   const atoms = createAtoms(iterations);
   const {getSnapshot} = getHelpers();
-  begin();
-  for (const node of atoms) {
-    // Set node to avoid hitting cached snapshots
-    setNode(node, 'SET');
-    const snapshot = getSnapshot();
-    expect(getNodeValue(node)).toBe('SET');
-    expect(snapshot.getLoadable(node).contents).toBe('SET');
-  }
+  perf(() => {
+    for (const node of atoms) {
+      // Set node to avoid hitting cached snapshots
+      setNode(node, 'SET');
+      const snapshot = getSnapshot();
+      expect(getNodeValue(node)).toBe('SET');
+      expect(snapshot.getLoadable(node).contents).toBe('SET');
+    }
+  });
+});
+
+testPerf('get 1 selector with n dependencies', ({iterations, perf}) => {
+  const atoms = createAtoms(iterations);
+  perf(() => {
+    getNodeValue(waitForAll(atoms));
+  });
+});
+
+testPerf('get 1 selector with n dependencies n times', ({iterations, perf}) => {
+  const atoms = createAtoms(iterations);
+  perf(() => {
+    for (let i = 0; i < iterations; i++) {
+      getNodeValue(waitForAll(atoms));
+    }
+  });
 });
 
-testPerf('Selector dependencies', ({iterations, begin}) => {
+testPerf('get n selectors n times', ({iterations, perf}) => {
   const atoms = createAtoms(iterations);
-  begin();
-  getNodeValue(waitForAll(atoms));
+  const testFamily = selectorFamily({
+    key: 'PERF-getselectors',
+    get:
+      id =>
+      ({get}) =>
+        get(atoms[id]) + get(atoms[0]),
+  });
+  perf(() => {
+    for (let i = 0; i < iterations; i++) {
+      for (let j = 0; j < iterations; j++) {
+        getNodeValue(testFamily(i));
+      }
+    }
+  });
 });
+
+testPerf(
+  'get n selectors with n dependencies n times',
+  ({iterations, perf}) => {
+    const atoms = createAtoms(iterations);
+    const testFamily = selectorFamily({
+      key: 'PERF-getselectors',
+      get: () => () => waitForAll(atoms),
+    });
+    perf(() => {
+      for (let i = 0; i < iterations; i++) {
+        for (let j = 0; j < iterations; j++) {
+          getNodeValue(testFamily(i));
+        }
+      }
+    });
+  },
+);

packages/recoil/recoil_values/Recoil_atom.js

@@ -108,13 +108,18 @@ export type PersistenceSettings<Stored> = $ReadOnly<{
   validator: (mixed, DefaultValue) => Stored | DefaultValue,
 }>;
 
-// TODO Support Loadable<T> and WrappedValue<T>
-type NewValue<T> = T | DefaultValue | Promise<T | DefaultValue>;
+// TODO Support Loadable<T>
+type NewValue<T> =
+  | T
+  | DefaultValue
+  | Promise<T | DefaultValue>
+  | WrappedValue<T>;
 type NewValueOrUpdater<T> =
   | T
   | DefaultValue
   | Promise<T | DefaultValue>
-  | ((T | DefaultValue) => T | DefaultValue);
+  | WrappedValue<T>
+  | ((T | DefaultValue) => T | DefaultValue | WrappedValue<T>);
 
 // Effect is called the first time a node is used with a <RecoilRoot>
 export type AtomEffect<T> = ({
@@ -127,7 +132,8 @@ export type AtomEffect<T> = ({
     | T
     | DefaultValue
     | Promise<T | DefaultValue>
-    | ((T | DefaultValue) => T | DefaultValue),
+    | WrappedValue<T>
+    | ((T | DefaultValue) => T | DefaultValue | WrappedValue<T>),
   ) => void,
   resetSelf: () => void,
 
@@ -167,6 +173,9 @@ type BaseAtomOptions<T> = $ReadOnly<{
   default: Promise<T> | Loadable<T> | WrappedValue<T> | T,
 }>;
 
+const unwrap = <T, S = T>(x: T | S | WrappedValue<T>): T | S =>
+  x instanceof WrappedValue ? x.value : x;
+
 function baseAtom<T>(options: BaseAtomOptions<T>): RecoilState<T> {
   const {key, persistence_UNSTABLE: persistence} = options;
 
@@ -193,11 +202,7 @@ function baseAtom<T>(options: BaseAtomOptions<T>): RecoilState<T> {
     ? options.default.state === 'loading'
       ? unwrapPromise((options.default: LoadingLoadableType<T>).contents)
       : options.default
-    : loadableWithValue(
-        options.default instanceof WrappedValue
-          ? options.default.value
-          : options.default,
-      );
+    : loadableWithValue(unwrap(options.default));
   maybeFreezeValueOrPromise(defaultLoadable.contents);
 
   let cachedAnswerForUnvalidatedValue: void | Loadable<T> = undefined;
@@ -280,8 +285,8 @@ function baseAtom<T>(options: BaseAtomOptions<T>): RecoilState<T> {
     const effects = options.effects ?? options.effects_UNSTABLE;
     if (effects != null) {
       // This state is scoped by Store, since this is in the initAtom() closure
-      let duringInit = true;
       let initValue: NewValue<T> = DEFAULT_VALUE;
+      let isDuringInit = true;
       let isInitError: boolean = false;
       let pendingSetSelf: ?{
         effect: AtomEffect<T>,
@@ -292,7 +297,7 @@ function baseAtom<T>(options: BaseAtomOptions<T>): RecoilState<T> {
         // Normally we can just get the current value of another atom.
         // But for our own value we need to check if there is a pending
         // initialized value or get the fallback default value.
-        if (duringInit && recoilValue.key === key) {
+        if (isDuringInit && recoilValue.key === key) {
           // Cast T to S
           const retValue: NewValue<S> = (initValue: any); // flowlint-line unclear-type:off
           return retValue instanceof DefaultValue
@@ -323,7 +328,7 @@ function baseAtom<T>(options: BaseAtomOptions<T>): RecoilState<T> {
           store.getState().nextTree ?? store.getState().currentTree,
           recoilValue.key,
         );
-        return duringInit &&
+        return isDuringInit &&
           recoilValue.key === key &&
           !(initValue instanceof DefaultValue)
           ? {...info, isSet: true, loadable: getLoadable(recoilValue)}
@@ -332,7 +337,7 @@ function baseAtom<T>(options: BaseAtomOptions<T>): RecoilState<T> {
 
       const setSelf =
         (effect: AtomEffect<T>) => (valueOrUpdater: NewValueOrUpdater<T>) => {
-          if (duringInit) {
+          if (isDuringInit) {
             const currentLoadable = getLoadable(node);
             const currentValue: T | DefaultValue =
               currentLoadable.state === 'hasValue'
@@ -356,21 +361,25 @@ function baseAtom<T>(options: BaseAtomOptions<T>): RecoilState<T> {
             }
 
             if (typeof valueOrUpdater !== 'function') {
-              pendingSetSelf = {effect, value: valueOrUpdater};
+              pendingSetSelf = {
+                effect,
+                value: unwrap<T, DefaultValue>(valueOrUpdater),
+              };
             }
 
             setRecoilValue(
               store,
               node,
               typeof valueOrUpdater === 'function'
                 ? currentValue => {
-                    const newValue =
+                    const newValue = unwrap(
                       // cast to any because we can't restrict T from being a function without losing support for opaque types
-                      (valueOrUpdater: any)(currentValue); // flowlint-line unclear-type:off
+                      (valueOrUpdater: any)(currentValue), // flowlint-line unclear-type:off
+                    );
                     pendingSetSelf = {effect, value: newValue};
                     return newValue;
                   }
-                : valueOrUpdater,
+                : unwrap(valueOrUpdater),
             );
           }
         };
@@ -447,17 +456,17 @@ function baseAtom<T>(options: BaseAtomOptions<T>): RecoilState<T> {
         }
       }
 
-      duringInit = false;
+      isDuringInit = false;
 
       // Mutate initial state in place since we know there are no other subscribers
       // since we are the ones initializing on first use.
       if (!(initValue instanceof DefaultValue)) {
-        const frozenInitValue = maybeFreezeValueOrPromise(initValue);
         const initLoadable = isInitError
           ? loadableWithError(initValue)
-          : isPromise(frozenInitValue)
-          ? loadableWithPromise(wrapPendingPromise(store, frozenInitValue))
-          : loadableWithValue(frozenInitValue);
+          : isPromise(initValue)
+          ? loadableWithPromise(wrapPendingPromise(store, initValue))
+          : loadableWithValue(unwrap(initValue));
+        maybeFreezeValueOrPromise(initLoadable.contents);
         initState.atomValues.set(key, initLoadable);
 
         // If there is a pending transaction, then also mutate the next state tree.
@@ -610,9 +619,7 @@ function atom<T>(options: AtomOptions<T>): RecoilState<T> {
   // @fb-only: ) {
     // @fb-only: return scopedAtom<T>({
       // @fb-only: ...restOptions,
-      // @fb-only: default: optionsDefault instanceof WrappedValue
-        // @fb-only: ? optionsDefault.value
-        // @fb-only: : optionsDefault,
+      // @fb-only: default: unwrap<T>(optionsDefault),
       // @fb-only: scopeRules_APPEND_ONLY_READ_THE_DOCS,
     // @fb-only: });
   } else {