feat(cache-map): Promote makeLRUCacheMap into a new package by gibson042 · Pull Request #2822 · endojs/endo

gibson042 · 2025-05-22T18:31:39Z

Description

Introduce @endo/cache-map.
Remove "LRU" eviction policy constraint (but keep that as the actual current implementation).
Support opting in to strongly-held keys (to be used in agoric-sdk for caching string-keyed board mappings).
Add a getMetrics() facet for evaluating cache efficiency (currently rudimentary).

Security Considerations

This package sits beneath SES, and so must be defensive without relying upon harden/assert/etc.

Scaling Considerations

The implementation remains a doubly-linked ring with permanent head and LRU eviction policy. All operations are O(1), mutating up to five nodes for promotion/insertion/removal (the target node and its former and new neighbors) but minimizing that when possible (e.g., bailing out early in moveCellAfter when the target is already in the right position). Cells are created up to the specified capacity and never destroyed, although their single-entry weak maps are necessarily replaced when evicting a tail in set. Cells are currently created lazily, although that could be made eager for better memory locality (probably with a boolean lazy option for opting out). All in all, it's probably about as good as an object-based implementation can be (and a bit better than the makeLRUCacheMap from which it is derived).

Documentation Considerations

Includes a README.md with explanation and example usage.

Testing Considerations

Existing tests have been ported and expanded. A future improvement could add GC assertions.

Compatibility Considerations

This continues to support SES, just now as a package that can additionally be used elsewhere.

Upgrade Considerations

None known; this is a new package.

gibson042 · 2025-05-22T18:32:40Z

packages/cache-map/types.d.ts

@@ -0,0 +1 @@
+export * from './src/cachemap.js';


Am I doing this right?

I dunno about endo these days, but I think agoric-sdk does it different. In general I don't like explicit types conditions in package.json, and prefer tsc to generate the .d.ts files as siblings much as possible.

@boneskull might have some more feedback here.

I’ve commandeered the branch and aligned this with the new policy.

And it transpires that the types.d.ts was doing a better job of exporting the types than the implicit index.d.ts so I have gone back to @gibson042’s original solution in hopes that we can find a better resolution eventually.

gibson042 · 2025-05-22T18:35:55Z

packages/cache-map/src/cachemap.js

+  // * liveTouchStats/evictedTouchStats { count, sum, mean, min, max }
+  //   * p50/p90/p95/p99 via Ben-Haim/Tom-Tov streaming histograms


A former colleague referred to count/sum/mean/min/max/p50/p90/p95/p99/p99.9 as the "big ten" metrics¹.

Footnotes

I believe this is the result of that influence. ↩

They’re pretty ubiquitous, to be sure.

kriskowal

I wish to rely on @erights review for the implementation. The packaging and compatibility story are in order.

Please change the version number to 1.0.0. When this lands, I can do a one-off publish so Agoric integration CI will continue without interruption.

packages/cache-map/package.json

mhofman

This looks reasonable.

Someday we'll have to make this durable compatible somehow, but we can cross that bridge then (an explicit linked list may not be appropriate in that case)

packages/cache-map/src/cachemap.js

packages/cache-map/test/index.test.js

mhofman · 2025-05-29T05:25:44Z

packages/cache-map/types.d.ts

@@ -0,0 +1 @@
+export * from './src/cachemap.js';


I dunno about endo these days, but I think agoric-sdk does it different. In general I don't like explicit types conditions in package.json, and prefer tsc to generate the .d.ts files as siblings much as possible.

@boneskull might have some more feedback here.

gibson042 · 2025-05-29T12:05:24Z

Please change the version number to 1.0.0. When this lands, I can do a one-off publish so Agoric integration CI will continue without interruption.

Done. @kriskowal this should be ready to merge, which I'd like to leave in your hands for that initial publish.

packages/cache-map/test/index.test.js

mhofman · 2025-05-31T00:22:34Z

packages/cache-map/src/cachemap.js

+      throw Error('internal: Unexpected non-singular cell data entry count');
+    }
+    const cellData = /** @type {Map<K, V>} */ (cell.data);
+    oldKey = /** @type {K} */ (cellData.keys().next().value);


Just thought of an alternative, not sure which is better

Suggested change

oldKey = /** @type {K} */ (cellData.keys().next().value);

([oldKey] = cellData.keys());

But regardless I'm also wondering if we shouldn't just call cellData.clear() and not fall through?

I originally had something like that, and it took me a minute to remember why I switched away so I added a comment explaining the paranoia. But I definitely want to use delete(oldKey) rather than clear(), primarily to use the bare minimum of the non-weak Map API—to which end, I've now made explicit that SingleEntryMap extends WeakMapAPI by at most size and keys.

Thinking more of it, I would prefer if the minimum WeakMap-ish API was an optional clear() instead of a keys(). The former is technically implementable by a something that keeps weak keys, where the latter isn't.

Was there a reason to prefer keys over clear ?

Okay, I'm convinced. Done.

boneskull

nothing blocking. I care most about the macro type 😜

boneskull · 2025-06-03T01:13:59Z

packages/cache-map/package.json

+  "name": "@endo/cache-map",
+  "version": "1.0.0",
+  "description": "bounded-size caches having WeakMap-compatible methods",
+  "keywords": [],


might wanna add smth here

I'm open to ideas.

"cache", "lru", "weakmap", etc. 😄

boneskull · 2025-06-03T01:14:40Z

packages/cache-map/package.json

+    "files": [
+      "test/**/*.test.*"
+    ],
+    "timeout": "2m"


I realize this is probably the default in skel, but I imagine this could be significantly reduced 😄

packages/cache-map/test/index.test.js

boneskull · 2025-06-03T01:29:26Z

packages/cache-map/test/index.test.js

+/**
+ * @param {import('ava').ExecutionContext} t
+ * @param {object} config
+ * @param {any} [config.makeMap]
+ * @param {string} config.expectTag
+ * @param {[unknown, unknown, unknown]} config.keys
+ * @param {unknown[]} [config.badKeys]
+ */


This should be a docstring on the function definition on L15, not on the macro itself.

suggestions:

Might be nicer to just use any instead of unknown to avoid extra type assertions

Use @import {x} from 'y' instead of import(y).x

ExecutionContext is generic on a context object, so you might want to use @template [T=unknown] / ExecutionContext<T>

There are a lot of assertions here; consider using t.plan()

I took the middle two bullet-point suggestions, but moving the docstring produced a lot of errors that I'd rather not deal with.

I'll take a look after merge

packages/cache-map/src/cachemap.js

boneskull · 2025-06-03T01:35:25Z

packages/cache-map/src/cachemap.js

+
+  /** @type {WeakMapAPI<K, CacheMapCell<K, V>>} */
+  const keyToCell = makeMap();
+  // @ts-expect-error this sentinel head is special


make a type for it?

I'd rather not; this is a singular exception.

packages/cache-map/src/cachemap.js

gibson042 · 2025-06-06T05:38:30Z

@kriskowal this should again be ready to merge, possibly pending re-review from @boneskull.

boneskull

@gibson042 LGTM

packages/cache-map/package.json

… redundant operations

…EntryMap

gibson042 requested review from erights, kriskowal and mhofman May 22, 2025 18:31

gibson042 commented May 22, 2025

View reviewed changes

kriskowal reviewed May 23, 2025

View reviewed changes

packages/cache-map/package.json Outdated Show resolved Hide resolved

mhofman requested a review from michaelfig May 24, 2025 00:58

gibson042 mentioned this pull request May 27, 2025

Harden before Lockdown #2782

Closed

mhofman approved these changes May 29, 2025

View reviewed changes

gibson042 force-pushed the gibson-2025-05-cache-map branch from dfba2e1 to 582f79f Compare May 29, 2025 12:42

mhofman reviewed May 29, 2025

View reviewed changes

packages/cache-map/test/index.test.js Show resolved Hide resolved

packages/cache-map/test/index.test.js Outdated Show resolved Hide resolved

mhofman approved these changes May 30, 2025

View reviewed changes

mhofman reviewed May 31, 2025

View reviewed changes

gibson042 force-pushed the gibson-2025-05-cache-map branch from 3017f39 to e4d7a80 Compare May 31, 2025 15:38

gibson042 closed this May 31, 2025

gibson042 reopened this May 31, 2025

gibson042 requested a review from boneskull June 2, 2025 15:29

kriskowal force-pushed the gibson-2025-05-cache-map branch 3 times, most recently from da41c9b to d916f3b Compare June 3, 2025 01:32

boneskull approved these changes Jun 3, 2025

View reviewed changes

kriskowal force-pushed the gibson-2025-05-cache-map branch from d916f3b to 3403bee Compare June 3, 2025 01:51

mhofman reviewed Jun 3, 2025

View reviewed changes

packages/cache-map/src/cachemap.js Outdated Show resolved Hide resolved

gibson042 force-pushed the gibson-2025-05-cache-map branch from 3403bee to 0dc492e Compare June 6, 2025 05:34

boneskull approved these changes Jun 6, 2025

View reviewed changes

kriskowal added the next-release label Jun 18, 2025

kriskowal approved these changes Jul 11, 2025

View reviewed changes

packages/cache-map/package.json Outdated Show resolved Hide resolved

packages/cache-map/package.json Outdated Show resolved Hide resolved

feat(cache-map): Promote makeLRUCacheMap into a new package

a39db9e

gibson042 added 8 commits July 11, 2025 13:27

refactor(cache-map): Relax "LRU" specificity

d899bcd

perf(cache-map): Clean up the implementation to reuse cells and avoid…

a5c3cf9

… redundant operations

feat(cache-map): Support creation of caches with strongly-held keys

6671bc8

feat(cache-map): Return a kit for exposing metrics

f513ebd

test(cache-map): Cover capacity={0,1} special cases

94a36e7

refactor(cache-map): Better leverage SingleEntryMap

fc7e372

refactor(cache-map): Use clear rather than keys to reset a Single…

64f352f

…EntryMap

chore(cache-map): DRY out method signatures

9e3fa1c

gibson042 force-pushed the gibson-2025-05-cache-map branch from adf8a20 to 9e3fa1c Compare July 11, 2025 17:27

gibson042 merged commit 97a538e into master Jul 11, 2025
29 of 30 checks passed

gibson042 deleted the gibson-2025-05-cache-map branch July 11, 2025 17:47

		// * liveTouchStats/evictedTouchStats { count, sum, mean, min, max }
		// * p50/p90/p95/p99 via Ben-Haim/Tom-Tov streaming histograms

	oldKey = /** @type {K} */ (cellData.keys().next().value);
	([oldKey] = cellData.keys());

Conversation

gibson042 commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Compatibility Considerations

Upgrade Considerations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kriskowal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhofman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibson042 commented May 29, 2025

Uh oh!

Uh oh!

Uh oh!

mhofman May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibson042 May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boneskull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boneskull Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gibson042 commented May 22, 2025 •

edited

Loading

mhofman May 31, 2025 •

edited

Loading

gibson042 May 31, 2025 •

edited

Loading

boneskull Jun 6, 2025 •

edited

Loading