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
Open

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

Changes from all 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
7 changes: 4 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,8 +89,6 @@ 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
Expand All @@ -99,7 +97,10 @@ const capacityOfDefaultCache = 50;
const makeDefaultCacheMap = weakKey =>
/** @type {WeakMapAPI<K, V>} */ (
makeCacheMapKit(capacityOfDefaultCache, {
makeMap: weakKey ? WeakMap : Map,
// We use a Map even for weakKey as the assumption is that we run under
// liveslots which virtualizes WeakMap, and since the mapping is
// bidirectional by default, the key would be pinned anyway.
makeMap: weakKey ? Map : Map,
Comment on lines +100 to +103
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.

This is still unsatisfying. Why do we need a parameter whose value is irrelevant? The function is not even exported from packages/internal/src/marshal/wrap-marshaller.js. Suggestion:

/**
 * Construct a cache map with default capacity, backed by a non-weak Map because
 * a) we assume execution under liveslots, in which WeakMap is virtualized, and
 * b) the mapping is bidirectional by default and thus would pin keys anyway.
 * @template K
 * @template V
 */
const makeDefaultCacheMap = () =>
  /** @type {WeakMapAPI<K, V>} */ (
    makeCacheMapKit(capacityOfDefaultCache, {
      makeMap: Map,
    }).cache
  );

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.

I would like the ability to simply revert to weak keys in the future without having to change the callsite.

Copy link
Member

@gibson042 gibson042 Nov 15, 2025

Choose a reason for hiding this comment

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

That seems like an anti-goal to me; clarity is far more important and there's no clear condition on when such a change would be effected. But if you can come up with good explanatory comment text, it might work out.

}).cache
);

Expand Down
Loading