From a6421cd8d1ae9e374f48beb6a138be1349512de3 Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Thu, 18 Jan 2024 13:59:15 -0500 Subject: [PATCH 1/3] refactor(ses): Move makeLRUCacheMap to a dedicated file It should eventually be an endo export, possibly in its own package (just not in `common`, which itself depends upon `ses`). --- packages/ses/src/error/note-log-args.js | 206 +---------------- packages/ses/src/make-lru-cachemap.js | 211 ++++++++++++++++++ packages/ses/test/error/test-note-log-args.js | 6 +- .../test/{error => }/test-lru-cache-map.js | 2 +- 4 files changed, 215 insertions(+), 210 deletions(-) create mode 100644 packages/ses/src/make-lru-cachemap.js rename packages/ses/test/{error => }/test-lru-cache-map.js (87%) diff --git a/packages/ses/src/error/note-log-args.js b/packages/ses/src/error/note-log-args.js index 2c764fe251..19ee35a2a0 100644 --- a/packages/ses/src/error/note-log-args.js +++ b/packages/ses/src/error/note-log-args.js @@ -2,216 +2,12 @@ /* eslint-disable @endo/no-polymorphic-call */ /* eslint-disable no-restricted-globals */ +import { makeLRUCacheMap } from '../make-lru-cachemap.js'; import './internal-types.js'; const { freeze } = Object; const { isSafeInteger } = Number; -/** - * @template Data - * @typedef {object} DoublyLinkedCell - * A cell of a doubly-linked ring, i.e., a doubly-linked circular list. - * DoublyLinkedCells are not frozen, and so should be closely encapsulated by - * any abstraction that uses them. - * @property {DoublyLinkedCell} next - * @property {DoublyLinkedCell} prev - * @property {Data} data - */ - -/** - * Makes a new self-linked cell. There are two reasons to do so: - * * To make the head sigil of a new initially-empty doubly-linked ring. - * * To make a non-sigil cell to be `spliceAfter`ed. - * - * @template Data - * @param {Data} data - * @returns {DoublyLinkedCell} - */ -const makeSelfCell = data => { - /** @type {Partial>} */ - const incompleteCell = { - next: undefined, - prev: undefined, - data, - }; - const selfCell = /** @type {DoublyLinkedCell} */ (incompleteCell); - selfCell.next = selfCell; - selfCell.prev = selfCell; - // Not frozen! - return selfCell; -}; - -/** - * Splices a self-linked non-sigil cell into a ring after `prev`. - * `prev` could be the head sigil, or it could be some other non-sigil - * cell within a ring. - * - * @template Data - * @param {DoublyLinkedCell} prev - * @param {DoublyLinkedCell} selfCell - */ -const spliceAfter = (prev, selfCell) => { - if (prev === selfCell) { - throw TypeError('Cannot splice a cell into itself'); - } - if (selfCell.next !== selfCell || selfCell.prev !== selfCell) { - throw TypeError('Expected self-linked cell'); - } - const cell = selfCell; - // rename variable cause it isn't self-linked after this point. - - const next = prev.next; - cell.prev = prev; - cell.next = next; - prev.next = cell; - next.prev = cell; - // Not frozen! - return cell; -}; - -/** - * @template Data - * @param {DoublyLinkedCell} cell - * No-op if the cell is self-linked. - */ -const spliceOut = cell => { - const { prev, next } = cell; - prev.next = next; - next.prev = prev; - cell.prev = cell; - cell.next = cell; -}; - -/** - * The LRUCacheMap is used within the implementation of `assert` and so - * at a layer below SES or harden. Thus, we give it a `WeakMap`-like interface - * rather than a `WeakMapStore`-like interface. To work before `lockdown`, - * the implementation must use `freeze` manually, but still exhaustively. - * - * It implements the WeakMap interface, and holds its keys weakly. Cached - * values are only held while the key is held by the user and the key/value - * bookkeeping cell has not been pushed off the end of the cache by `budget` - * number of more recently referenced cells. If the key is dropped by the user, - * the value will no longer be held by the cache, but the bookkeeping cell - * itself will stay in memory. - * - * @template {{}} K - * @template {unknown} V - * @param {number} keysBudget - * @returns {WeakMap} - */ -export const makeLRUCacheMap = keysBudget => { - if (!isSafeInteger(keysBudget) || keysBudget < 0) { - throw TypeError('keysBudget must be a safe non-negative integer number'); - } - /** @typedef {DoublyLinkedCell | undefined>} LRUCacheCell */ - /** @type {WeakMap} */ - const keyToCell = new WeakMap(); - let size = 0; // `size` must remain <= `keysBudget` - // As a sigil, `head` uniquely is not in the `keyToCell` map. - /** @type {LRUCacheCell} */ - const head = makeSelfCell(undefined); - - const touchCell = key => { - const cell = keyToCell.get(key); - if (cell === undefined || cell.data === undefined) { - // Either the key was GCed, or the cell was condemned. - return undefined; - } - // Becomes most recently used - spliceOut(cell); - spliceAfter(head, cell); - return cell; - }; - - /** - * @param {K} key - */ - const has = key => touchCell(key) !== undefined; - freeze(has); - - /** - * @param {K} key - */ - // UNTIL https://github.com/endojs/endo/issues/1514 - // Prefer: const get = key => touchCell(key)?.data?.get(key); - const get = key => { - const cell = touchCell(key); - return cell && cell.data && cell.data.get(key); - }; - freeze(get); - - /** - * @param {K} key - * @param {V} value - */ - const set = (key, value) => { - if (keysBudget < 1) { - // eslint-disable-next-line no-use-before-define - return lruCacheMap; // Implements WeakMap.set - } - - let cell = touchCell(key); - if (cell === undefined) { - cell = makeSelfCell(undefined); - spliceAfter(head, cell); // start most recently used - } - if (!cell.data) { - // Either a fresh cell or a reused condemned cell. - size += 1; - // Add its data. - cell.data = new WeakMap(); - // Advertise the cell for this key. - keyToCell.set(key, cell); - while (size > keysBudget) { - const condemned = head.prev; - spliceOut(condemned); // Drop least recently used - condemned.data = undefined; - size -= 1; - } - } - - // Update the data. - cell.data.set(key, value); - - // eslint-disable-next-line no-use-before-define - return lruCacheMap; // Implements WeakMap.set - }; - freeze(set); - - // "delete" is a keyword. - /** - * @param {K} key - */ - const deleteIt = key => { - const cell = keyToCell.get(key); - if (cell === undefined) { - return false; - } - spliceOut(cell); - keyToCell.delete(key); - if (cell.data === undefined) { - // Already condemned. - return false; - } - - cell.data = undefined; - size -= 1; - return true; - }; - freeze(deleteIt); - - const lruCacheMap = freeze({ - has, - get, - set, - delete: deleteIt, - [Symbol.toStringTag]: 'LRUCacheMap', - }); - return lruCacheMap; -}; -freeze(makeLRUCacheMap); - const defaultLoggedErrorsBudget = 1000; const defaultArgsPerErrorBudget = 100; diff --git a/packages/ses/src/make-lru-cachemap.js b/packages/ses/src/make-lru-cachemap.js new file mode 100644 index 0000000000..1cc7d1f544 --- /dev/null +++ b/packages/ses/src/make-lru-cachemap.js @@ -0,0 +1,211 @@ +// @ts-check +/* eslint-disable @endo/no-polymorphic-call */ +/* eslint-disable no-restricted-globals */ + +const { freeze } = Object; +const { isSafeInteger } = Number; + +/** + * @template Data + * @typedef {object} DoublyLinkedCell + * A cell of a doubly-linked ring, i.e., a doubly-linked circular list. + * DoublyLinkedCells are not frozen, and so should be closely encapsulated by + * any abstraction that uses them. + * @property {DoublyLinkedCell} next + * @property {DoublyLinkedCell} prev + * @property {Data} data + */ + +/** + * Makes a new self-linked cell. There are two reasons to do so: + * * To make the head sigil of a new initially-empty doubly-linked ring. + * * To make a non-sigil cell to be `spliceAfter`ed. + * + * @template Data + * @param {Data} data + * @returns {DoublyLinkedCell} + */ +const makeSelfCell = data => { + /** @type {Partial>} */ + const incompleteCell = { + next: undefined, + prev: undefined, + data, + }; + const selfCell = /** @type {DoublyLinkedCell} */ (incompleteCell); + selfCell.next = selfCell; + selfCell.prev = selfCell; + // Not frozen! + return selfCell; +}; + +/** + * Splices a self-linked non-sigil cell into a ring after `prev`. + * `prev` could be the head sigil, or it could be some other non-sigil + * cell within a ring. + * + * @template Data + * @param {DoublyLinkedCell} prev + * @param {DoublyLinkedCell} selfCell + */ +const spliceAfter = (prev, selfCell) => { + if (prev === selfCell) { + throw TypeError('Cannot splice a cell into itself'); + } + if (selfCell.next !== selfCell || selfCell.prev !== selfCell) { + throw TypeError('Expected self-linked cell'); + } + const cell = selfCell; + // rename variable cause it isn't self-linked after this point. + + const next = prev.next; + cell.prev = prev; + cell.next = next; + prev.next = cell; + next.prev = cell; + // Not frozen! + return cell; +}; + +/** + * @template Data + * @param {DoublyLinkedCell} cell + * No-op if the cell is self-linked. + */ +const spliceOut = cell => { + const { prev, next } = cell; + prev.next = next; + next.prev = prev; + cell.prev = cell; + cell.next = cell; +}; + +/** + * The LRUCacheMap is used within the implementation of `assert` and so + * at a layer below SES or harden. Thus, we give it a `WeakMap`-like interface + * rather than a `WeakMapStore`-like interface. To work before `lockdown`, + * the implementation must use `freeze` manually, but still exhaustively. + * + * It implements the WeakMap interface, and holds its keys weakly. Cached + * values are only held while the key is held by the user and the key/value + * bookkeeping cell has not been pushed off the end of the cache by `budget` + * number of more recently referenced cells. If the key is dropped by the user, + * the value will no longer be held by the cache, but the bookkeeping cell + * itself will stay in memory. + * + * @template {{}} K + * @template {unknown} V + * @param {number} keysBudget + * @returns {WeakMap} + */ +export const makeLRUCacheMap = keysBudget => { + if (!isSafeInteger(keysBudget) || keysBudget < 0) { + throw TypeError('keysBudget must be a safe non-negative integer number'); + } + /** @typedef {DoublyLinkedCell | undefined>} LRUCacheCell */ + /** @type {WeakMap} */ + const keyToCell = new WeakMap(); + let size = 0; // `size` must remain <= `keysBudget` + // As a sigil, `head` uniquely is not in the `keyToCell` map. + /** @type {LRUCacheCell} */ + const head = makeSelfCell(undefined); + + const touchCell = key => { + const cell = keyToCell.get(key); + if (cell === undefined || cell.data === undefined) { + // Either the key was GCed, or the cell was condemned. + return undefined; + } + // Becomes most recently used + spliceOut(cell); + spliceAfter(head, cell); + return cell; + }; + + /** + * @param {K} key + */ + const has = key => touchCell(key) !== undefined; + freeze(has); + + /** + * @param {K} key + */ + // UNTIL https://github.com/endojs/endo/issues/1514 + // Prefer: const get = key => touchCell(key)?.data?.get(key); + const get = key => { + const cell = touchCell(key); + return cell && cell.data && cell.data.get(key); + }; + freeze(get); + + /** + * @param {K} key + * @param {V} value + */ + const set = (key, value) => { + if (keysBudget < 1) { + // eslint-disable-next-line no-use-before-define + return lruCacheMap; // Implements WeakMap.set + } + + let cell = touchCell(key); + if (cell === undefined) { + cell = makeSelfCell(undefined); + spliceAfter(head, cell); // start most recently used + } + if (!cell.data) { + // Either a fresh cell or a reused condemned cell. + size += 1; + // Add its data. + cell.data = new WeakMap(); + // Advertise the cell for this key. + keyToCell.set(key, cell); + while (size > keysBudget) { + const condemned = head.prev; + spliceOut(condemned); // Drop least recently used + condemned.data = undefined; + size -= 1; + } + } + + // Update the data. + cell.data.set(key, value); + + // eslint-disable-next-line no-use-before-define + return lruCacheMap; // Implements WeakMap.set + }; + freeze(set); + + // "delete" is a keyword. + /** + * @param {K} key + */ + const deleteIt = key => { + const cell = keyToCell.get(key); + if (cell === undefined) { + return false; + } + spliceOut(cell); + keyToCell.delete(key); + if (cell.data === undefined) { + // Already condemned. + return false; + } + + cell.data = undefined; + size -= 1; + return true; + }; + freeze(deleteIt); + + const lruCacheMap = freeze({ + has, + get, + set, + delete: deleteIt, + [Symbol.toStringTag]: 'LRUCacheMap', + }); + return lruCacheMap; +}; +freeze(makeLRUCacheMap); diff --git a/packages/ses/test/error/test-note-log-args.js b/packages/ses/test/error/test-note-log-args.js index 683b43be39..c0f5b19f9e 100644 --- a/packages/ses/test/error/test-note-log-args.js +++ b/packages/ses/test/error/test-note-log-args.js @@ -1,10 +1,8 @@ // @ts-check import test from 'ava'; -import { - makeNoteLogArgsArrayKit, - makeLRUCacheMap, -} from '../../src/error/note-log-args.js'; +import { makeNoteLogArgsArrayKit } from '../../src/error/note-log-args.js'; +import { makeLRUCacheMap } from '../../src/make-lru-cachemap.js'; test('note log args array kit basic', t => { const { addLogArgs, takeLogArgsArray } = makeNoteLogArgsArrayKit(3, 2); diff --git a/packages/ses/test/error/test-lru-cache-map.js b/packages/ses/test/test-lru-cache-map.js similarity index 87% rename from packages/ses/test/error/test-lru-cache-map.js rename to packages/ses/test/test-lru-cache-map.js index d2c84dc6ff..27599d7cde 100644 --- a/packages/ses/test/error/test-lru-cache-map.js +++ b/packages/ses/test/test-lru-cache-map.js @@ -1,6 +1,6 @@ import test from 'ava'; -import { makeLRUCacheMap } from '../../src/error/note-log-args.js'; +import { makeLRUCacheMap } from '../src/make-lru-cachemap.js'; test('lru cache map basic', t => { const lruMap = makeLRUCacheMap(2); From 28569c864cb0e6a42c9a881a5d3df553c750fd94 Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Thu, 18 Jan 2024 14:47:02 -0500 Subject: [PATCH 2/3] test(ses): Consolidate makeLRUCacheMap testing --- packages/ses/test/error/test-note-log-args.js | 38 ---------- packages/ses/test/test-lru-cache-map.js | 72 ++++++++++++++++--- 2 files changed, 64 insertions(+), 46 deletions(-) diff --git a/packages/ses/test/error/test-note-log-args.js b/packages/ses/test/error/test-note-log-args.js index c0f5b19f9e..7de7d680f7 100644 --- a/packages/ses/test/error/test-note-log-args.js +++ b/packages/ses/test/error/test-note-log-args.js @@ -2,7 +2,6 @@ import test from 'ava'; import { makeNoteLogArgsArrayKit } from '../../src/error/note-log-args.js'; -import { makeLRUCacheMap } from '../../src/make-lru-cachemap.js'; test('note log args array kit basic', t => { const { addLogArgs, takeLogArgsArray } = makeNoteLogArgsArrayKit(3, 2); @@ -24,40 +23,3 @@ test('note log args array kit basic', t => { t.deepEqual(takeLogArgsArray(e3), undefined); t.deepEqual(takeLogArgsArray(e4), [['d']]); }); - -test('weak LRUCacheMap', t => { - /** @type {WeakMap<{}, number>} */ - const lru = makeLRUCacheMap(3); - const o1 = {}; - const o2 = {}; - const o3 = {}; - const o4 = {}; - - // Overflow drops the oldest. - lru.set(o1, 1); - t.is(lru.get(o1), 1); - lru.set(o3, 2); - lru.set(o2, 3); - lru.set(o4, 4); // drops o1 - t.falsy(lru.has(o1)); - t.is(lru.get(o4), 4); - lru.set(o4, 5); - t.is(lru.get(o4), 5); - t.true(lru.has(o4)); - lru.set(o1, 6); // drops o3 - t.is(lru.get(o1), 6); - t.false(lru.has(o3)); - - // Explicit delete keeps all other elements. - lru.delete(o1); // explicit delete o1 - t.is(lru.get(o1), undefined); - t.false(lru.has(o1)); - t.true(lru.has(o2)); - t.true(lru.has(o4)); - t.false(lru.has(o3)); - lru.set(o3, 7); - lru.set(o1, 8); // drops o2 - t.false(lru.has(o2)); - t.is(lru.get(o1), 8); - t.is(lru.get(o3), 7); -}); diff --git a/packages/ses/test/test-lru-cache-map.js b/packages/ses/test/test-lru-cache-map.js index 27599d7cde..4d346ea2dc 100644 --- a/packages/ses/test/test-lru-cache-map.js +++ b/packages/ses/test/test-lru-cache-map.js @@ -1,22 +1,78 @@ +// @ts-check import test from 'ava'; import { makeLRUCacheMap } from '../src/make-lru-cachemap.js'; -test('lru cache map basic', t => { +test('makeLRUCacheMap', t => { + /** @type {WeakMap<{}, number>} */ const lruMap = makeLRUCacheMap(2); + const assertNoEntry = key => { + t.is(lruMap.has(key), false); + t.is(lruMap.get(key), undefined); + }; + const assertEntry = (key, expectedValue) => { + t.is(lruMap.has(key), true); + t.is(lruMap.get(key), expectedValue); + }; const key1 = {}; const key2 = {}; const key3 = {}; - t.is(lruMap.has(key1), false); + assertNoEntry(key1); + // Populate up to capacity. lruMap.set(key1, 'x'); lruMap.set(key2, 'y'); - t.is(lruMap.has(key2), true); - t.is(lruMap.has(key1), true); - t.is(lruMap.has(key3), false); + assertEntry(key2, 'y'); + assertEntry(key1, 'x'); + assertNoEntry(key3); + // Evict key2. lruMap.set(key3, 'z'); - t.is(lruMap.has(key1), true); - t.is(lruMap.has(key2), false); - t.is(lruMap.has(key3), true); + assertEntry(key1, 'x'); + assertNoEntry(key2); + assertEntry(key3, 'z'); + + // Overwrite key3. + lruMap.set(key3, 'zz'); + assertEntry(key1, 'x'); + assertNoEntry(key2); + assertEntry(key3, 'zz'); + + // Evict key1. + lruMap.set(key2, 'y'); + assertNoEntry(key1); + assertEntry(key2, 'y'); + assertEntry(key3, 'zz'); + + // Delete key3, preserving key2. + lruMap.delete(key3); + assertNoEntry(key1); + assertEntry(key2, 'y'); + assertNoEntry(key3); + + // Add key1, preserving key2. + lruMap.set(key1, 'x'); + assertEntry(key2, 'y'); + assertEntry(key1, 'x'); + assertNoEntry(key3); + + // Delete key2, preserving key1. + lruMap.delete(key2); + assertEntry(key1, 'x'); + assertNoEntry(key2); + assertNoEntry(key3); + + // Delete key1. + lruMap.delete(key1); + assertNoEntry(key1); + assertNoEntry(key2); + assertNoEntry(key3); + + // Repopulate with eviction. + lruMap.set(key1, 'xx'); + lruMap.set(key2, 'yy'); + lruMap.set(key3, 'zz'); + assertNoEntry(key1); + assertEntry(key2, 'yy'); + assertEntry(key3, 'zz'); }); From 1adcc57798933a3c2843a9a26126ba466cf50570 Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Thu, 18 Jan 2024 14:47:47 -0500 Subject: [PATCH 3/3] chore(ses): Tighten makeLRUCacheMap linting --- packages/ses/src/make-lru-cachemap.js | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/ses/src/make-lru-cachemap.js b/packages/ses/src/make-lru-cachemap.js index 1cc7d1f544..67be58cf59 100644 --- a/packages/ses/src/make-lru-cachemap.js +++ b/packages/ses/src/make-lru-cachemap.js @@ -1,9 +1,12 @@ // @ts-check /* eslint-disable @endo/no-polymorphic-call */ -/* eslint-disable no-restricted-globals */ -const { freeze } = Object; +// eslint-disable-next-line no-restricted-globals const { isSafeInteger } = Number; +// eslint-disable-next-line no-restricted-globals +const { freeze } = Object; +// eslint-disable-next-line no-restricted-globals +const { toStringTag: toStringTagSymbol } = Symbol; /** * @template Data @@ -50,9 +53,11 @@ const makeSelfCell = data => { */ const spliceAfter = (prev, selfCell) => { if (prev === selfCell) { + // eslint-disable-next-line no-restricted-globals throw TypeError('Cannot splice a cell into itself'); } if (selfCell.next !== selfCell || selfCell.prev !== selfCell) { + // eslint-disable-next-line no-restricted-globals throw TypeError('Expected self-linked cell'); } const cell = selfCell; @@ -100,10 +105,12 @@ const spliceOut = cell => { */ export const makeLRUCacheMap = keysBudget => { if (!isSafeInteger(keysBudget) || keysBudget < 0) { + // eslint-disable-next-line no-restricted-globals throw TypeError('keysBudget must be a safe non-negative integer number'); } /** @typedef {DoublyLinkedCell | undefined>} LRUCacheCell */ /** @type {WeakMap} */ + // eslint-disable-next-line no-restricted-globals const keyToCell = new WeakMap(); let size = 0; // `size` must remain <= `keysBudget` // As a sigil, `head` uniquely is not in the `keyToCell` map. @@ -158,6 +165,7 @@ export const makeLRUCacheMap = keysBudget => { // Either a fresh cell or a reused condemned cell. size += 1; // Add its data. + // eslint-disable-next-line no-restricted-globals cell.data = new WeakMap(); // Advertise the cell for this key. keyToCell.set(key, cell); @@ -204,7 +212,9 @@ export const makeLRUCacheMap = keysBudget => { get, set, delete: deleteIt, - [Symbol.toStringTag]: 'LRUCacheMap', + // eslint-disable-next-line jsdoc/check-types + [/** @type {typeof Symbol.toStringTag} */ (toStringTagSymbol)]: + 'LRUCacheMap', }); return lruCacheMap; };