From 0f3015e124a870f46a10f2e227c64673dcdcdecd Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Wed, 31 Jul 2024 11:06:29 -0400 Subject: [PATCH 1/4] style(SwingSet): Refactor gc-actions.js for comprehensibility * Add doc comments * Add typings * Rename some functions to better indicate semantics * Remove an unused parameter --- packages/SwingSet/src/kernel/gc-actions.js | 48 +++++++++++++++------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/packages/SwingSet/src/kernel/gc-actions.js b/packages/SwingSet/src/kernel/gc-actions.js index 01adbeeff45..27d81b64fbe 100644 --- a/packages/SwingSet/src/kernel/gc-actions.js +++ b/packages/SwingSet/src/kernel/gc-actions.js @@ -3,8 +3,11 @@ import { insistKernelType } from './parseKernelSlots.js'; import { insistVatID } from '../lib/id.js'; /** + * @typedef {`v${number}`} VatID + * @typedef {`ko${number}`} KOID * @typedef {'dropExport' | 'retireExport' | 'retireImport'} GCActionType * @typedef {'dropExports' | 'retireExports' | 'retireImports'} GCQueueEventType + * @typedef {`${VatID} ${GCActionType} ${KOID}`} GCActionString */ /** @@ -25,8 +28,13 @@ const queueTypeFromActionType = new Map([ ['retireImport', 'retireImports'], ]); +/** + * @param {GCActionString} s + */ function parseAction(s) { - const [vatID, type, kref] = s.split(' '); + const [vatID, type, kref] = /** @type {[VatID, GCActionType, KOID]} */ ( + s.split(' ') + ); insistVatID(vatID); queueTypeFromActionType.has(type) || Fail`unknown type ${type}`; insistKernelType('object', kref); @@ -62,11 +70,16 @@ export function processGCActionSet(kernelKeeper) { // we must bypass the action as redundant (since it's an error to delete // the same c-list entry twice). - // This `filterAction` function looks at each queued GC Action and decides - // whether the current state of the c-lsits and reference counts warrants - // permits the action to run, or if it should be negated/bypassed. - - function filterAction(vatKeeper, action, type, kref) { + /** + * Inspect a queued GC action and decide whether the current state of c-lists + * and reference counts warrants processing it, or if it should instead be + * negated/bypassed. + * + * @param {import('../types-external.js').VatKeeper} vatKeeper + * @param {GCActionType} type + * @param {KOID} kref + */ + function shouldProcessAction(vatKeeper, type, kref) { const hasCList = vatKeeper.hasCListEntry(kref); const isReachable = hasCList ? vatKeeper.getReachableFlag(kref) : undefined; const exists = kernelKeeper.kernelObjectExists(kref); @@ -107,12 +120,16 @@ export function processGCActionSet(kernelKeeper) { // may need to change to support that, to ensure that `dropExport` and // `retireExport` can both be delivered. - function filterActions(vatID, groupedActions) { + /** + * @param {VatID} vatID + * @param {GCActionString[]} groupedActions + */ + function krefsToProcess(vatID, groupedActions) { const vatKeeper = kernelKeeper.provideVatKeeper(vatID); const krefs = []; for (const action of groupedActions) { const { type, kref } = parseAction(action); - if (filterAction(vatKeeper, action, type, kref)) { + if (shouldProcessAction(vatKeeper, type, kref)) { krefs.push(kref); } allActionsSet.delete(action); @@ -121,6 +138,7 @@ export function processGCActionSet(kernelKeeper) { return krefs; } + /** @type {Map>} */ const grouped = new Map(); // grouped.get(vatID).get(type) = krefs to process for (const action of allActionsSet) { const { vatID, type } = parseAction(action); @@ -142,28 +160,28 @@ export function processGCActionSet(kernelKeeper) { for (const type of actionTypePriorities) { if (forVat.has(type)) { const actions = forVat.get(type); - const krefs = filterActions(vatID, actions); + const krefs = krefsToProcess(vatID, actions); if (krefs.length) { // at last, we act krefs.sort(); // remove the work we're about to do from the durable set kernelKeeper.setGCActions(allActionsSet); - const queueType = /** @type {GCQueueEventType} */ ( - queueTypeFromActionType.get(type) - ); + const queueType = queueTypeFromActionType.get(type); return harden({ type: queueType, vatID, krefs }); } } } } + if (actionSetUpdated) { // remove negated items from the durable set kernelKeeper.setGCActions(allActionsSet); - // return a special gc-nop event to tell kernel to record our + // return a special event to tell kernel to record our // DB changes in their own crank return harden({ type: 'negated-gc-action', vatID: undefined }); - } else { - return undefined; // no GC work to do and no DB changes } + + // no GC work to do and no DB changes + return undefined; } harden(processGCActionSet); From ee85ee73af4b6d8f6a21bca7c0c3902955a54bc0 Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Wed, 31 Jul 2024 11:21:37 -0400 Subject: [PATCH 2/4] style(SwingSet): Rename some variables in gc-actions.js --- packages/SwingSet/src/kernel/gc-actions.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/SwingSet/src/kernel/gc-actions.js b/packages/SwingSet/src/kernel/gc-actions.js index 27d81b64fbe..3262ec26edd 100644 --- a/packages/SwingSet/src/kernel/gc-actions.js +++ b/packages/SwingSet/src/kernel/gc-actions.js @@ -139,27 +139,27 @@ export function processGCActionSet(kernelKeeper) { } /** @type {Map>} */ - const grouped = new Map(); // grouped.get(vatID).get(type) = krefs to process + const actionsByVat = new Map(); for (const action of allActionsSet) { const { vatID, type } = parseAction(action); - if (!grouped.has(vatID)) { - grouped.set(vatID, new Map()); + if (!actionsByVat.has(vatID)) { + actionsByVat.set(vatID, new Map()); } - const forVat = grouped.get(vatID); - if (!forVat.has(type)) { - forVat.set(type, []); + const actionsForVatByType = actionsByVat.get(vatID); + if (!actionsForVatByType.has(type)) { + actionsForVatByType.set(type, []); } - forVat.get(type).push(action); + actionsForVatByType.get(type).push(action); } - const vatIDs = Array.from(grouped.keys()); + const vatIDs = Array.from(actionsByVat.keys()); vatIDs.sort(); for (const vatID of vatIDs) { - const forVat = grouped.get(vatID); + const actionsForVatByType = actionsByVat.get(vatID); // find the highest-priority type of work to do within this vat for (const type of actionTypePriorities) { - if (forVat.has(type)) { - const actions = forVat.get(type); + if (actionsForVatByType.has(type)) { + const actions = actionsForVatByType.get(type); const krefs = krefsToProcess(vatID, actions); if (krefs.length) { // at last, we act From fde19d0cddbd000dde3c341b16b045d3a3692485 Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Wed, 31 Jul 2024 11:50:31 -0400 Subject: [PATCH 3/4] chore(internal): Define TotalMap and TotalMapFrom> types Ref https://github.com/microsoft/TypeScript/issues/9619 --- packages/internal/src/storage-test-utils.js | 15 +-------------- packages/internal/src/types.d.ts | 11 +++++++++++ 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/packages/internal/src/storage-test-utils.js b/packages/internal/src/storage-test-utils.js index 9da583ab10a..b1ce51b2cdc 100644 --- a/packages/internal/src/storage-test-utils.js +++ b/packages/internal/src/storage-test-utils.js @@ -9,25 +9,12 @@ import { bindAllMethods } from './method-tools.js'; import { eventLoopIteration } from './testing-utils.js'; /** + * @import {TotalMap} from './types.js'; * @import {Marshaller, StorageEntry, StorageMessage, StorageNode} from './lib-chainStorage.js'; */ const trace = makeTracer('StorTU', false); -/** - * A map corresponding with a total function such that `get(key)` is assumed to - * always succeed. - * - * @template K, V - * @typedef {{ [k in Exclude, 'get'>]: Map[k] } & { - * get: (key: K) => V; - * }} TotalMap - */ -/** - * @template T - * @typedef {T extends Map ? TotalMap : never} TotalMapFrom - */ - /** * A convertSlotToVal function that produces basic Remotables. Assumes that all * slots are Remotables (i.e. none are Promises). diff --git a/packages/internal/src/types.d.ts b/packages/internal/src/types.d.ts index 806e5882cf5..2cbeb768137 100644 --- a/packages/internal/src/types.d.ts +++ b/packages/internal/src/types.d.ts @@ -3,6 +3,17 @@ import type { ERef, RemotableBrand } from '@endo/eventual-send'; import type { Primitive } from '@endo/pass-style'; import type { Callable } from './utils.js'; +/** + * A map corresponding with a total function such that `get(key)` is assumed to + * always succeed. + */ +export type TotalMap = Omit, 'get'> & { + /** Returns the element associated with the specified key in the TotalMap. */ + get: (key: K) => V; +}; +export type TotalMapFrom = + M extends Map ? TotalMap : never; + export declare class Callback any> { private iface: I; From ae923d73132ce00e240ccf0e1f88b99608cafce7 Mon Sep 17 00:00:00 2001 From: Richard Gibson Date: Wed, 31 Jul 2024 11:51:30 -0400 Subject: [PATCH 4/4] chore(SwingSet): Use TotalMap to avoid TypeScript complaints --- packages/SwingSet/src/kernel/gc-actions.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/SwingSet/src/kernel/gc-actions.js b/packages/SwingSet/src/kernel/gc-actions.js index 3262ec26edd..ca575525169 100644 --- a/packages/SwingSet/src/kernel/gc-actions.js +++ b/packages/SwingSet/src/kernel/gc-actions.js @@ -2,6 +2,8 @@ import { Fail } from '@endo/errors'; import { insistKernelType } from './parseKernelSlots.js'; import { insistVatID } from '../lib/id.js'; +/** @import {TotalMap} from '@agoric/internal'; */ + /** * @typedef {`v${number}`} VatID * @typedef {`ko${number}`} KOID @@ -19,14 +21,15 @@ const actionTypePriorities = ['dropExport', 'retireExport', 'retireImport']; /** * A mapping of GC action type to queue event type. - * - * @type {Map} */ -const queueTypeFromActionType = new Map([ - ['dropExport', 'dropExports'], - ['retireExport', 'retireExports'], - ['retireImport', 'retireImports'], -]); +const queueTypeFromActionType = + /** @type {TotalMap} */ ( + new Map([ + ['dropExport', 'dropExports'], + ['retireExport', 'retireExports'], + ['retireImport', 'retireImports'], + ]) + ); /** * @param {GCActionString} s @@ -138,7 +141,7 @@ export function processGCActionSet(kernelKeeper) { return krefs; } - /** @type {Map>} */ + /** @type {TotalMap>} */ const actionsByVat = new Map(); for (const action of allActionsSet) { const { vatID, type } = parseAction(action);