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

Remove unnecessary promiseRegistrations #10778

Open
mhofman opened this issue Dec 27, 2024 · 0 comments
Open

Remove unnecessary promiseRegistrations #10778

mhofman opened this issue Dec 27, 2024 · 0 comments
Assignees
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes

Comments

@mhofman
Copy link
Member

mhofman commented Dec 27, 2024

What is the Problem Being Solved?

While investigating the growing heap of v43-walletFactory (#10706), we identified then worked around a liveslots/virtualReferenceManager leak through watchPromise's promiseRegistrations. While we did the minimal possible fix in #10758 and plan to fix virtual reference leak in #10757, it appears that we don't need the virtual promiseRegistrations Map in the first place.

#5514 introduced the scalar map to make sure the promise would stay in slotToVal even if it wasn't exported. However 1) I believe that it wasn't actually needed since we'd never have collected it anyway (part of the bug we want to fix in #10757) and 2) we've since changed the logic to make sure the promise is exported if locally decided. As such the promise will maintain its vpid association while the kernel pillar is maintained, aka until it settles (either locally or remotely). And even then, the settle handler doesn't seem to be relying on the mapping of the promise object to vpid anyway since it closes over the vpid and doesn't use the promise identity at all.

Description of the Design

Replace promiseRegistrations scalar MapStore by a heap Set used only to keep track of promises watched during buildRootObject before loadWatchedPromiseTable is called (to avoid reviving promises newly watched)

Security Considerations

None

Scaling Considerations

Removes a bunch of unnecessary virtual data syscalls.

Test Plan

Existing test coverage should be sufficient. Make sure it covers promises watched and possibly settled before buildRootObject completes.

Upgrade Considerations

Liveslots code change so it requires a chain software upgrade and an upgrade of vats using watchPromise.

@mhofman mhofman added enhancement New feature or request liveslots requires vat-upgrade to deploy changes labels Dec 27, 2024
@mhofman mhofman self-assigned this Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request liveslots requires vat-upgrade to deploy changes
Projects
None yet
Development

No branches or pull requests

1 participant