Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[simple-cache-provider] Use LRU cache eviction #12851

Merged
merged 2 commits into from
May 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
147 changes: 116 additions & 31 deletions packages/simple-cache-provider/src/SimpleCacheProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,43 +17,60 @@ const Pending = 1;
const Resolved = 2;
const Rejected = 3;

type EmptyRecord = {|
type EmptyRecord<K, V> = {|
status: 0,
suspender: null,
key: K,
value: null,
error: null,
next: Record<K, V> | null,
previous: Record<K, V> | null,
|};

type PendingRecord<V> = {|
type PendingRecord<K, V> = {|
status: 1,
suspender: Promise<V>,
suspender: Promise<K, V>,
key: K,
value: null,
error: null,
next: Record<K, V> | null,
previous: Record<K, V> | null,
|};

type ResolvedRecord<V> = {|
type ResolvedRecord<K, V> = {|
status: 2,
suspender: null,
key: K,
value: V,
error: null,
next: Record<K, V> | null,
previous: Record<K, V> | null,
|};

type RejectedRecord = {|
type RejectedRecord<K, V> = {|
status: 3,
suspender: null,
key: K,
value: null,
error: Error,
next: Record<K, V> | null,
previous: Record<K, V> | null,
|};

type Record<V> =
| EmptyRecord
| PendingRecord<V>
| ResolvedRecord<V>
| RejectedRecord;
type Record<K, V> =
| EmptyRecord<K, V>
| PendingRecord<K, V>
| ResolvedRecord<K, V>
| RejectedRecord<K, V>;

type RecordCache<K, V> = {|
map: Map<K, Record<K, V>>,
head: Record<K, V> | null,
tail: Record<K, V> | null,
|};

type RecordCache<K, V> = Map<K, Record<V>>;
// TODO: How do you express this type with Flow?
type ResourceCache = Map<any, RecordCache<any, any>>;
type ResourceMap = Map<any, RecordCache<any, any>>;
type Cache = {
invalidate(): void,
read<K, V, A>(
Expand Down Expand Up @@ -86,10 +103,35 @@ if (__DEV__) {
value.$$typeof === CACHE_TYPE;
}

// TODO: Make this configurable per resource
const MAX_SIZE = 500;
const PAGE_SIZE = 50;

function createRecord<K, V>(key: K): EmptyRecord<K, V> {
return {
status: Empty,
suspender: null,
key,
value: null,
error: null,
next: null,
previous: null,
};
}

function createRecordCache<K, V>(): RecordCache<K, V> {
return {
map: new Map(),
head: null,
tail: null,
size: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than keeping and updating the 'size' property here, could we rely on the size property of the map attribute of the cache?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to land this PR, we can fix these in a follow-up.

};
}

export function createCache(invalidator: () => mixed): Cache {
const resourceCache: ResourceCache = new Map();
const resourceMap: ResourceMap = new Map();

function getRecord<K, V>(resourceType: any, key: K): Record<V> {
function accessRecord<K, V>(resourceType: any, key: K): Record<V> {
if (__DEV__) {
warning(
typeof resourceType !== 'string' && typeof resourceType !== 'number',
Expand All @@ -100,25 +142,68 @@ export function createCache(invalidator: () => mixed): Cache {
);
}

let recordCache = resourceCache.get(resourceType);
if (recordCache !== undefined) {
const record = recordCache.get(key);
if (record !== undefined) {
return record;
let recordCache = resourceMap.get(resourceType);
if (recordCache === undefined) {
recordCache = createRecordCache();
resourceMap.set(resourceType, recordCache);
}
const map = recordCache.map;

let record = map.get(key);
if (record === undefined) {
// This record does not already exist. Create a new one.
record = createRecord(key);
map.set(key, record);
if (recordCache.size >= MAX_SIZE) {
// The cache is already at maximum capacity. Remove PAGE_SIZE least
// recently used records.
// TODO: We assume the max capcity is greater than zero. Otherwise warn.
const tail = recordCache.tail;
if (tail !== null) {
let newTail = tail;
for (let i = 0; i < PAGE_SIZE && newTail !== null; i++) {
recordCache.size -= 1;
map.delete(newTail.key);
newTail = newTail.previous;
}
recordCache.tail = newTail;
if (newTail !== null) {
newTail.next = null;
}
}
}
} else {
// This record is already cached. Remove it from its current position in
// the list. We'll add it to the front below.
const previous = record.previous;
const next = record.next;
if (previous !== null) {
previous.next = next;
} else {
recordCache.head = next;
}
if (next !== null) {
next.previous = previous;
} else {
recordCache.tail = previous;
}
recordCache.size -= 1;
}

// Add the record to the front of the list.
const head = recordCache.head;
const newHead = record;
recordCache.head = newHead;
newHead.previous = null;
newHead.next = head;
if (head !== null) {
head.previous = newHead;
} else {
recordCache = new Map();
resourceCache.set(resourceType, recordCache);
recordCache.tail = newHead;
}
recordCache.size += 1;

const record = {
status: Empty,
suspender: null,
value: null,
error: null,
};
recordCache.set(key, record);
return record;
return newHead;
}

function load<V>(emptyRecord: EmptyRecord, suspender: Promise<V>) {
Expand Down Expand Up @@ -154,7 +239,7 @@ export function createCache(invalidator: () => mixed): Cache {
miss: A => Promise<V>,
missArg: A,
): void {
const record: Record<V> = getRecord(resourceType, key);
const record: Record<V> = accessRecord(resourceType, key);
switch (record.status) {
case Empty:
// Warm the cache.
Expand All @@ -178,7 +263,7 @@ export function createCache(invalidator: () => mixed): Cache {
miss: A => Promise<V>,
missArg: A,
): V {
const record: Record<V> = getRecord(resourceType, key);
const record: Record<V> = accessRecord(resourceType, key);
switch (record.status) {
case Empty:
// Load the requested resource.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,4 +189,41 @@ describe('SimpleCacheProvider', () => {
fn();
}
});

it('stays within maximum capacity by evicting the least recently used record', async () => {
const {createCache, createResource} = SimpleCacheProvider;

function loadIntegerString(int) {
return Promise.resolve(int + '');
}
const IntegerStringResource = createResource(loadIntegerString);
const cache = createCache();

// TODO: This is hard-coded to a maximum size of 500. Make this configurable
// per resource.
for (let n = 1; n <= 500; n++) {
IntegerStringResource.preload(cache, n);
}

// Access 1, 2, and 3 again. The least recently used integer is now 4.
IntegerStringResource.preload(cache, 3);
IntegerStringResource.preload(cache, 2);
IntegerStringResource.preload(cache, 1);

// Evict older integers from the cache by adding new ones.
IntegerStringResource.preload(cache, 501);
IntegerStringResource.preload(cache, 502);
IntegerStringResource.preload(cache, 503);

await Promise.resolve();

// 1, 2, and 3 should be in the cache. 4, 5, and 6 should have been evicted.
expect(IntegerStringResource.read(cache, 1)).toEqual('1');
expect(IntegerStringResource.read(cache, 2)).toEqual('2');
expect(IntegerStringResource.read(cache, 3)).toEqual('3');

expect(() => IntegerStringResource.read(cache, 4)).toThrow(Promise);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one thing - I think even if 1, 2, and 3 are also evicted I think this will still pass. We could verify that those don't throw Promises?

expect(() => IntegerStringResource.read(cache, 5)).toThrow(Promise);
expect(() => IntegerStringResource.read(cache, 6)).toThrow(Promise);
});
});