Skip to content

Commit

Permalink
Earlier selector cache lookup (#1720)
Browse files Browse the repository at this point in the history 
Summary:
Pull Request resolved: #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
  • Loading branch information
@drarmstr @facebook-github-bot
drarmstr authored and facebook-github-bot committed Apr 13, 2022
1 parent 685c36b commit 854e574
Show file tree
Hide file tree
Showing 5 changed files with 192 additions and 91 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
## UPCOMING
**_Add new changes here as they land_**

- Selector cache lookup optimization (#1720)

## 0.7.1 (2022-04-12)

### Typing
Expand Down
29 changes: 16 additions & 13 deletions packages/recoil/core/Recoil_Snapshot.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
130 changes: 99 additions & 31 deletions packages/recoil/core/__tests__/Recoil_perf-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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});
});
}

Expand Down Expand Up @@ -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));
}
}
});
},
);
76 changes: 32 additions & 44 deletions packages/recoil/recoil_values/Recoil_selector.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,32 +318,6 @@ function selector<T>(
stores.add(store);
}

function getCachedNodeLoadable<TT>(
store: Store,
state: TreeState,
nodeKey: NodeKey,
): Loadable<TT> {
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
Expand Down Expand Up @@ -707,7 +681,7 @@ function selector<T>(
setDepsInStore(store, state, deps, executionId);
}

const depLoadable = getCachedNodeLoadable(store, state, depKey);
const depLoadable = getNodeLoadable(store, state, depKey);

depValues.set(depKey, depLoadable);

Expand Down Expand Up @@ -803,12 +777,18 @@ function selector<T>(
store: Store,
state: TreeState,
): ?Loadable<T> {
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',
Expand All @@ -820,7 +800,7 @@ function selector<T>(
{
onNodeVisit: node => {
if (node.type === 'branch' && node.nodeKey !== key) {
depsAfterCacheDone.add(node.nodeKey);
depsAfterCacheLookup.add(node.nodeKey);
}
},
},
Expand All @@ -831,24 +811,28 @@ function selector<T>(
);
}

/**
* 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;
}

/**
Expand Down Expand Up @@ -1083,6 +1067,10 @@ function selector<T>(
}

function selectorPeek(store: Store, state: TreeState): ?Loadable<T> {
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;
Expand Down Expand Up @@ -1125,7 +1113,7 @@ function selector<T>(
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;
Expand Down
Loading

0 comments on commit 854e574

Please sign in to comment.