diff --git a/CHANGELOG.md b/CHANGELOG.md index 08f9ed2fb..515ffaf9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ ## UPCOMING **_Add new changes here as they land_** +- Selector cache lookup optimization (#1720) + ## 0.7.1 (2022-04-12) ### Typing diff --git a/packages/recoil/core/Recoil_Snapshot.js b/packages/recoil/core/Recoil_Snapshot.js index 891d8bf12..af58269ac 100644 --- a/packages/recoil/core/Recoil_Snapshot.js +++ b/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, diff --git a/packages/recoil/core/__tests__/Recoil_perf-test.js b/packages/recoil/core/__tests__/Recoil_perf-test.js index 7a5a338c0..fe9dd1635 100644 --- a/packages/recoil/core/__tests__/Recoil_perf-test.js +++ b/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)); + } + } + }); + }, +); diff --git a/packages/recoil/recoil_values/Recoil_selector.js b/packages/recoil/recoil_values/Recoil_selector.js index 2554f85a9..9de73559b 100644 --- a/packages/recoil/recoil_values/Recoil_selector.js +++ b/packages/recoil/recoil_values/Recoil_selector.js @@ -318,32 +318,6 @@ function selector( stores.add(store); } - function getCachedNodeLoadable( - store: Store, - state: TreeState, - nodeKey: NodeKey, - ): Loadable { - const isKeyPointingToSelector = store - .getState() - .knownSelectors.has(nodeKey); - - /** - * It's important that we don't bypass calling getNodeLoadable for atoms - * as getNodeLoadable has side effects in state - */ - if (isKeyPointingToSelector && state.atomValues.has(nodeKey)) { - return nullthrows(state.atomValues.get(nodeKey)); - } - - const loadable = getNodeLoadable(store, state, nodeKey); - - if (loadable.state !== 'loading' && isKeyPointingToSelector) { - state.atomValues.set(nodeKey, loadable); - } - - return loadable; - } - /** * This function attaches a then() and a catch() to a promise that was * returned from a selector's get() (either explicitly or implicitly by @@ -707,7 +681,7 @@ function selector( setDepsInStore(store, state, deps, executionId); } - const depLoadable = getCachedNodeLoadable(store, state, depKey); + const depLoadable = getNodeLoadable(store, state, depKey); depValues.set(depKey, depLoadable); @@ -803,12 +777,18 @@ function selector( store: Store, state: TreeState, ): ?Loadable { - const depsAfterCacheDone = new Set(); - const executionInfo = getExecutionInfo(store); + // First, look up in the state cache + // If it's here, then the deps in the store should already be valid. + let cachedLoadable = state.atomValues.get(key); + if (cachedLoadable != null) { + return cachedLoadable; + } - let cachedVal; + // Second, look up in the selector cache and update the deps in the store + const depsAfterCacheLookup = new Set(); + const executionInfo = getExecutionInfo(store); try { - cachedVal = cache.get( + cachedLoadable = cache.get( nodeKey => { invariant( typeof nodeKey === 'string', @@ -820,7 +800,7 @@ function selector( { onNodeVisit: node => { if (node.type === 'branch' && node.nodeKey !== key) { - depsAfterCacheDone.add(node.nodeKey); + depsAfterCacheLookup.add(node.nodeKey); } }, }, @@ -831,24 +811,28 @@ function selector( ); } - /** - * Ensure store contains correct dependencies if we hit the cache so that - * the store deps and cache are in sync for a given state. This is important - * because store deps are normally updated when new executions are created, - * but cache hits don't trigger new executions but they still _may_ signifiy - * a change in deps in the store if the store deps for this state are empty - * or stale. - */ - if (cachedVal) { + if (cachedLoadable) { + // Cache the results in the state to allow for cheaper lookup than + // iterating the tree cache of dependencies. + state.atomValues.set(key, cachedLoadable); + + /** + * Ensure store contains correct dependencies if we hit the cache so that + * the store deps and cache are in sync for a given state. This is important + * because store deps are normally updated when new executions are created, + * but cache hits don't trigger new executions but they still _may_ signify + * a change in deps in the store if the store deps for this state are empty + * or stale. + */ setDepsInStore( store, state, - depsAfterCacheDone, + depsAfterCacheLookup, executionInfo?.latestExecutionId, ); } - return cachedVal; + return cachedLoadable; } /** @@ -1083,6 +1067,10 @@ function selector( } function selectorPeek(store: Store, state: TreeState): ?Loadable { + const cachedLoadable = state.atomValues.get(key); + if (cachedLoadable != null) { + return cachedLoadable; + } return cache.get(nodeKey => { invariant(typeof nodeKey === 'string', 'Cache nodeKey is type string'); return peekNodeLoadable(store, state, nodeKey)?.contents; @@ -1125,7 +1113,7 @@ function selector( throw err('Recoil: Async selector sets are not currently supported.'); } - const loadable = getCachedNodeLoadable(store, state, depKey); + const loadable = getNodeLoadable(store, state, depKey); if (loadable.state === 'hasValue') { return loadable.contents; diff --git a/packages/recoil/recoil_values/__tests__/Recoil_atom-test.js b/packages/recoil/recoil_values/__tests__/Recoil_atom-test.js index 4d8473156..aa9641764 100644 --- a/packages/recoil/recoil_values/__tests__/Recoil_atom-test.js +++ b/packages/recoil/recoil_values/__tests__/Recoil_atom-test.js @@ -1214,6 +1214,48 @@ describe('Effects', () => { expect(c.textContent).toEqual('"OTHER""INIT"'); }); + testRecoil( + 'atom effect runs twice when atom is read from a snapshot and the atom is read for first time in that snapshot', + ({strictMode, concurrentMode}) => { + let numTimesEffectInit = 0; + let latestSetSelf = a => a; + + const atomWithEffect = atom({ + key: 'atomWithEffect', + default: 0, + effects: [ + ({setSelf}) => { + latestSetSelf = setSelf; + setSelf(1); // to accurately reproduce minimal reproducible example based on GitHub #1107 issue + numTimesEffectInit++; + }, + ], + }); + + const Component = () => { + const readSelFromSnapshot = useRecoilCallback(({snapshot}) => () => { + snapshot.getLoadable(atomWithEffect); + }); + + readSelFromSnapshot(); // first initialization; + + return useRecoilValue(atomWithEffect); // second initialization; + }; + + const c = renderElements(); + expect(c.textContent).toBe('1'); + expect(numTimesEffectInit).toBe(strictMode && concurrentMode ? 3 : 2); + + act(() => latestSetSelf(100)); + expect(c.textContent).toBe('100'); + expect(numTimesEffectInit).toBe(strictMode && concurrentMode ? 3 : 2); + + act(() => latestSetSelf(200)); + expect(c.textContent).toBe('200'); + expect(numTimesEffectInit).toBe(strictMode && concurrentMode ? 3 : 2); + }, + ); + /** * See github issue #1107 item #1 */ @@ -1229,9 +1271,7 @@ describe('Effects', () => { effects: [ ({setSelf}) => { latestSetSelf = setSelf; - - setSelf(1); // to accurately reproduce minimal reproducible example based on GitHub issue - + setSelf(1); // to accurately reproduce minimal reproducible example based on GitHub #1107 issue numTimesEffectInit++; }, ],