Skip to content

fix(internal): use a plain Map as the backing cache for wrapped marshaller #12195

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+4 −3
Open

fix(internal): use a plain Map as the backing cache for wrapped marshaller #12195

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
20ce60c
fix(internal): use a plain Map as the backing cache for wrapped marsh…
mhofman Nov 7, 2025
d6b0f21
fixup! fix(internal): use a plain Map as the backing cache for wrappe…
mhofman Nov 7, 2025
2d15474
fixup! fix(internal): use a plain Map as the backing cache for wrappe…
mhofman Nov 10, 2025
f1ee069
Merge branch 'master'
mhofman Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/internal/src/marshal/wrap-marshaller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,16 @@ const { slotToWrapper, wrapperToSlot } = (() => {

const capacityOfDefaultCache = 50;

// TODO(https://github.com/Agoric/agoric-sdk/issues/12111)
// Check cost of using virtual-aware WeakMap in liveslots
/**
* @template K
* @template V
* @param {boolean} [weakKey]
*/
// eslint-disable-next-line no-unused-vars
const makeDefaultCacheMap = weakKey =>
/** @type {WeakMapAPI<K, V>} */ (
makeCacheMapKit(capacityOfDefaultCache, {
makeMap: weakKey ? WeakMap : Map,
makeMap: Map,
}).cache
);
Copy link
Member

@gibson042 gibson042 Nov 10, 2025

Choose a reason for hiding this comment

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

We shouldn't be ignoring a parameter; either drop it from the signature or leave this function as-is and update the call sites to explicitly opt-out of weak keying.

Copy link
Member Author

@mhofman mhofman Nov 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The call site needs to pass weakKey for non-default maker, and as I provide a defaut makeCacheMap implementation, it needs to accept a parameter (or it'd be a type error). I could prefix the param with _ to make the eslint error go away, but I kinda dislike that in general.

Would a comment explaining why I'm ignoring the param satisfy your concern?

Not awake, there is no makeCacheMap.

Copy link
Member Author

@mhofman mhofman Nov 10, 2025

Choose a reason for hiding this comment

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

Kept the ternary and added a comment


Expand Down
Loading