From 854e574197c72aa9492ea47576bd7688e2b4e921 Mon Sep 17 00:00:00 2001 From: Douglas Armstrong Date: Wed, 13 Apr 2022 00:28:42 -0700 Subject: [PATCH] Earlier selector cache lookup (#1720) Summary: Pull Request resolved: https://github.com/facebookexperimental/Recoil/pull/1720 More red lines! While supporting some users I realized an opportunity to simplify the selector implementation slightly and optimize cache lookups to happen earlier. Previously, selectors would not cache their own value in the store state, only their dependencies. So, repeated lookups of a selector would require looking up in the more expensive selector tree cache. This optimization fixes that. It also makes `Snapshot` clones more robust by always cloning the `TreeState` to avoid snapshot selector lookup aching to inadvertently affect the parent store, which may cause atoms to skip initialization now that we are more aggresively caching. * Performance test of reading 100 selectors 100 times each took 1/3rd the time. * Performance test of reading 100 selectors with 100 dependencies 100 times each took 1/3rd the time. Reviewed By: noritheduck Differential Revision: D35492328 fbshipit-source-id: 89547b97a7f61b03153cf3afe366a6f47ef22e52 --- CHANGELOG.md | 2 + packages/recoil/core/Recoil_Snapshot.js | 29 ++-- .../recoil/core/__tests__/Recoil_perf-test.js | 130 +++++++++++++----- .../recoil/recoil_values/Recoil_selector.js | 76 +++++----- .../__tests__/Recoil_atom-test.js | 46 ++++++- 5 files changed, 192 insertions(+), 91 deletions(-) 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++; }, ],