fix(liveslots): avoid slotToVal memory leak for watched promises #10758
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
warner
added
SwingSet
Deploying agoric-sdk with
|
| Latest commit: |
874196c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fd81452b.agoric-sdk.pages.dev |
| Branch Preview URL: | https://10756-watch-promise-leak.agoric-sdk.pages.dev |
toliaqat
approved these changes
Dec 20, 2024
gibson042
approved these changes
Dec 20, 2024
mhofman
approved these changes
Dec 20, 2024
Liveslots has a bug (#10757) which leaks slotToVal entries when a tracked Promise is still being held in virtual data (e.g. a merely-virtual MapStore) at the time it becomes settled. This is triggered by `watchPromise` because of the order in which we attach two handlers: one which notices the resolution and is inhibited from deleting the slotToVal entry, and a second which removes the Promise from the (virtual) `promiseRegistrations` collection (thus enabling the deletion). For any watched Promise that is resolved, we leave a `slotToVal` entry (with an empty WeakRef) in RAM until the end of the incarnation. This commit adds a test.failing to demonstrate the presence of the bug. Each time we watch and then resolve a promise, the slotToVal table grows by one entry. refs #10756
Liveslots has a bug (#10757) which leaks slotToVal entries when a tracked Promise is still being held in virtual data (e.g. a merely-virtual MapStore) at the time it becomes settled. This is triggered by `watchPromise` because of the order in which we attach two handlers: one which notices the resolution and is inhibited from deleting the slotToVal entry, and a second which removes the Promise from the (virtual) `promiseRegistrations` collection (thus enabling the deletion). For any watched Promise that is resolved, we leave a `slotToVal` entry (with an empty WeakRef) in RAM until the end of the incarnation. This PR does not fix the underlying bug, but it rearranges the handler order to avoid triggering it. closes #10756 refs #10706
warner
force-pushed
the
10756-watch-promise-leak
branch
from
bec3f6e to
874196c
Compare
mergify bot
added a commit
that referenced
this pull request
Dec 24, 2024
refs: #10758, #10756 closes: #10762 ## Description Upgrade the Orchestration (both Core and API) vats to use liveslots with the #10758 fix for a `watchPromise` memory leak (#10756). ### Security Considerations No code change to the vat user code in question, just to the liveslots it relies on. ### Scaling Considerations Should flatten the memory growth curve. ### Documentation Considerations No changes to existing data. ### Testing Considerations The upgraded vats should be monitored for memory growth. ### Upgrade Considerations n/a
This was referenced Dec 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Liveslots has a bug (#10757) which leaks slotToVal entries when a tracked Promise is still being held in virtual data (e.g. a merely-virtual MapStore) at the time it becomes settled. This is triggered by
watchPromisebecause of the order in which we attach two handlers: one which notices the resolution and is inhibited from deleting the slotToVal entry, and a second which removes the Promise from the (virtual)promiseRegistrationscollection (thus enabling the deletion). For any watched Promise that is resolved, we leave aslotToValentry (with an empty WeakRef) in RAM until the end of the incarnation.This PR does not fix the underlying bug, but it rearranges the handler order to avoid triggering it.
The attached unit test fails with the original handler order (
slotToVal.sizegrows), and passes with the swapped order (slotToVal.sizeremains constant).closes #10756
refs #10706