Skip to content
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.

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

vat-environment passStyleOf cache holds strong references to objects with vrefs #9781

Closed
gibson042 opened this issue Jul 25, 2024 · 1 comment · Fixed by #9874
Closed

vat-environment passStyleOf cache holds strong references to objects with vrefs #9781

gibson042 opened this issue Jul 25, 2024 · 1 comment · Fixed by #9874
Labels
enhancement New feature or request

Comments

@gibson042
Copy link
Member

What is the Problem Being Solved?

Importing passStyleOf from endo results in the function being constructed on top of the virtual-object-aware WeakMap provided by liveslots, which holds strong references to objects with vrefs for purposes of hiding GC events. But this means that it is not appropriate for use as the passStyleOf cache, which sees all such objects that pass into or out of the vat.

Description of the Design

We can instead let liveslots provide passStyleOf against the real WeakMap in such a way that the vat-environment import uses that endowment rather than rebuilding on top of the virtualized WeakMap. This is already in progress with coordination on the endo side.

Security Considerations

It is important to continue protecting the real WeakMap—wrappers such as passStyleOf are acceptable only to the extent that they do not expose GC-sensing powers, and as such must be considered on a case-by-case basis.

Scaling Considerations

This should significantly reduce RAM consumption by vats, dramatically so in the case of those that send and/or receive lots of Remotable-bearing messages (such as the board vat).

Test Plan

We'll definitely be measuring the approach, with as close an approximation as possible to the real vat environment.

Upgrade Considerations

Upgrade will be required to take advantage of the new endo and liveslots code.

@mhofman
Copy link
Member

mhofman commented Jul 27, 2024

Correction: the virtual aware weak collections provided by liveslots do NOT hold any references strongly on the key side, whether that's the virtual objects reference, or the "physical" representative. Instead it uses a Map keyed by the vref string, and some registrations to indicate these vrefs have a recognizer. Of course that could cause some references to be held strongly if cycles involving the value part of these weak collections were involved, but that's not the case for passStyle ones.

@mergify mergify bot closed this as completed in #9874 Aug 17, 2024
@mergify mergify bot closed this as completed in b6c58f2 Aug 17, 2024
erights added a commit to endojs/endo that referenced this issue Aug 22, 2024
…2408)

Closes: #2407 
Refs: Agoric/agoric-sdk#9874
Agoric/agoric-sdk#9431
Agoric/agoric-sdk#9781 #2377

## Description

While working on Agoric/agoric-sdk#9874 , I
found that it still failed to endow `passStyleOf` via a symbol-named
property as recently agree.

Turns out there was still a lurking `Object.keys` on the compartment
endowment pathway that needs to be changed to enumerate all enumerable
own properties, whether string-named or symbol-named.

### Security Considerations

None

### Scaling Considerations

For this PR, none. But it enables liveslots to endow its `passStyleOf`
to the compartments it makes, which has the scaling benefits explained
at Agoric/agoric-sdk#9781

### Documentation Considerations

For the normal developer, none. Once endowed, `passStyleOf` will have
performance that is less surprising.

### Testing Considerations

Agoric/agoric-sdk#9874 both patches in the
equivalent of this PR, uses it to endow `passStyleOf`, and test that it
has done so, across both node and XS.

However, after this fix, Agoric/agoric-sdk#9874
works on the Node host. But it still fails on the XS host for
undiagnosed reasons, so there may be another bug like #2407 still
lurking somewhere. Until diagnosed, it isn't clear if that remaining
problem is an endo problem or an agoric-sdk problem.

### Compatibility and Upgrade Considerations

Except for code meant only to test this, `passStyleOf` should not have
any observable difference. We have not attempted to make an other use of
symbol-named global properties, so there should be no compat or upgrade
issues.
kriskowal pushed a commit that referenced this issue Aug 27, 2024
…tment (#9874)

A variant of #9431 . @warner , feel free to just adopt these changes into #9431 rather than reviewing this alternate.

closes: #9781
refs: #9431 endojs/endo#2377 endojs/endo#2408

## Description

The code running under liveslots, i.e., user-level vat code such as contracts, must not be able to sense gc. Thus, liveslots endows them with virtual-storage-aware WeakMap and WeakSet, which treats the virtual object as the weakly held key, whereas the builtin WeakMap and WeakSet would treat the momentary representative as the weakly held key. To achieve this, the virtual-storage-aware WeakMap and WeakSet must impose a comparative storage leak.

However, some WeakMaps and WeakSets are used purely as an encapsulated unobservable memo, in the sense that the clients of encapsulating abstraction cannot sense whether the memo hit or missed (modulo timing of course, which we can also deny). `passStyleOf` is such an abstraction. Measurements show that the storage leak it causes is significant. The judgement of `passStyleOf` is only to report the pass-style of its arguments, and all virtual objects that have representative have a pass-style of `'remotable'` every time any of its representatives are tested.

To avoid this storage leak, endojs/endo#2377 (merged, released, and synced with agoric-sdk) and endojs/endo#2408 (still in review) together enable liveslots to endow the compartment it unbundles with its own efficient `passStyleOf`, built from the primitive WeakMap which it encapsulates.

This PR does two things:
- makes the change to liveslots to do this endowing, according to the conventions supported by endojs/endo#2377 and endojs/endo#2408
- because endojs/endo#2408 is not yet synced with agoric-sdk, this PR adds an "equivalent" patch, so that we can depend on it before the next endo sync.

### Security Considerations

This design *assumes* that the endowed `passStyleOf` makes the memo hits vs misses unobservable, so its dependence on these does not enable the code using it to observe gc. If there is some way to trick it into exposing the difference between a hit and miss, that would be a security concern.

### Scaling Considerations

The point.

With this PR, the storage leak caused by the `passStyleOf` memo should go away. For some vats, this should be a big improvement.

### Documentation Considerations

For the normal developer, none.

### Testing Considerations

Adapts the tests originally written by @warner in #9431 , which seem to demonstrate that this works both for node-based and for XS-based vats.

### Upgrade Considerations

I don't believe there are any. When linked with an endo preceding even endojs/endo#2377 , the only consequence should be that the storage leak remains unfixed. Likewise, if an endo with endojs/endo#2377 and even endojs/endo#2408 is linked with an agoric-sdk prior to the PR, the only consequence should be that the storage leak remains unfixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants