Skip to content

Commit

Permalink
style: Refactor gc-actions.js for comprehensibility (#9813)
Browse files Browse the repository at this point in the history 
## Description
Prompted by a question in Slack.
* Define internal TotalMap<K, V> and TotalMapFrom<Map<K, V>> types (cf. microsoft/TypeScript#9619 )
* Add doc comments
* Add typings
* Rename some functions and variables to better indicate semantics
* Remove an unused parameter

### Security Considerations
n/a

### Scaling Considerations
n/a

### Documentation Considerations
Automatically provides a better IDE experience.

### Testing Considerations
n/a

### Upgrade Considerations
n/a
  • Loading branch information
@mergify
mergify[bot] authored Jul 31, 2024
2 parents 20cb52f + ae923d7 commit b1d610d
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 47 deletions.
87 changes: 54 additions & 33 deletions packages/SwingSet/src/kernel/gc-actions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,14 @@ 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
* @typedef {'dropExport' | 'retireExport' | 'retireImport'} GCActionType
* @typedef {'dropExports' | 'retireExports' | 'retireImports'} GCQueueEventType
* @typedef {`${VatID} ${GCActionType} ${KOID}`} GCActionString
*/

/**
Expand All @@ -16,17 +21,23 @@ const actionTypePriorities = ['dropExport', 'retireExport', 'retireImport'];

/**
* A mapping of GC action type to queue event type.
*
* @type {Map<GCActionType, GCQueueEventType>}
*/
const queueTypeFromActionType = new Map([
['dropExport', 'dropExports'],
['retireExport', 'retireExports'],
['retireImport', 'retireImports'],
]);
const queueTypeFromActionType =
/** @type {TotalMap<GCActionType, GCQueueEventType>} */ (
new Map([
['dropExport', 'dropExports'],
['retireExport', 'retireExports'],
['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);
Expand Down Expand Up @@ -62,11 +73,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);
Expand Down Expand Up @@ -107,12 +123,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);
Expand All @@ -121,49 +141,50 @@ export function processGCActionSet(kernelKeeper) {
return krefs;
}

const grouped = new Map(); // grouped.get(vatID).get(type) = krefs to process
/** @type {TotalMap<VatID, TotalMap<GCActionType, GCActionString[]>>} */
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);
const krefs = filterActions(vatID, actions);
if (actionsForVatByType.has(type)) {
const actions = actionsForVatByType.get(type);
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);
15 changes: 1 addition & 14 deletions packages/internal/src/storage-test-utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<keyof Map<K, V>, 'get'>]: Map<K, V>[k] } & {
* get: (key: K) => V;
* }} TotalMap
*/
/**
* @template T
* @typedef {T extends Map<infer K, infer V> ? TotalMap<K, V> : never} TotalMapFrom
*/

/**
* A convertSlotToVal function that produces basic Remotables. Assumes that all
* slots are Remotables (i.e. none are Promises).
Expand Down
11 changes: 11 additions & 0 deletions packages/internal/src/types.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<K, V> = Omit<Map<K, V>, 'get'> & {
/** Returns the element associated with the specified key in the TotalMap. */
get: (key: K) => V;
};
export type TotalMapFrom<M extends Map> =
M extends Map<infer K, infer V> ? TotalMap<K, V> : never;

export declare class Callback<I extends (...args: unknown[]) => any> {
private iface: I;

Expand Down

0 comments on commit b1d610d

Please sign in to comment.