various comms-GC-related cleanups #3363
Merged
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
force-pushed
the
3306-cleanup
branch
3 times, most recently
from
20af6b9 to
7f18cff
Compare
dckc
reviewed
Jun 20, 2021
|
@FUDCo I added two more commits that warrant your attention, as they change the one-sided c-list retirement code we discussed many moons ago. |
There used to be code that called incrementRefCount / decrementRefCount with a message `.result` value, which could be `null` if the message was a sendOnly. All such code was changed to avoid calling inc/dec with an empty reference, so now inc/dec can be slightly simpler.
The return value was leftover from an earlier approach, and will make increasingly less sense as we introduce object-refcount checking.
This generalizes the refcounting code, which was previously only for
promises, into a form that's ready to track objects as well.
* replace `deadLocalPromises` with `maybeFree` (a set of lrefs)
* replace `purgeDeadLocalPromises()` with `processMaybeFree()`
* don't remove lrefs from the free list when incrementing, just
let processMaybeFree notice the count is no longer zero
* change the main loop in `processMaybeFree()`
* this ought to survive items being added to the set multiple times while
it runs (even though Sets can be modified during iteration, I think the
previous approach was vulnerable to losing items in unusual conditions)
* tolerate promises that were deleted from the table before processing
Errors during comms processing were not being logged very usefully, and are more likely to indicate a bug in the comms code than to be something provoked by userspace, so log any errors during `dispach()` before re-throwing them. Remove a number of helper functions in preparation for changes to the way GC deliveries are handled.
When the comms vat resolves a promise, it begins a two-part retirement process. Right after it sends the `resolve` message to the remote, it deletes the outbound half of the c-list, because this promise ID is partially retired, and the comms vat won't be referencing it in any outbound messages again. The inbound half is retained until the other side acknowledges the resolution message (i.e. some inbound messages arrives which demonstrates awareness of the `notify`, by including an `ackSeqNum` at least as large as the `seqNum` of the outbound `notify`). This changes the code to retain both sides of the c-list entry until the ack is received. I think this winds up being slightly cleaner, and most importantly it retains our ability to map local-ref to remote-ref, which enables an upcoming change to `deleteRemoteMapping` / `deleteKernelMapping` to only take a single argument (lref, not lref+rret), which is a lot cleaner, and avoids some error cases. refs #3306
The comms vat c-list management code has a pair of functions (`deleteRemoteMapping` and `deleteKernelMapping`) to remove c-list entries. These functions take both a local reference (lref) and an externally-facing reference (kfref for the kernel-facing side, rref for the remote-facing side). This change removes the externally-facing reference from the API, so now you call it with only the lref. Both functions look up the necessary reference internally. This removes an easy-to-make error from `deleteRemoteMapping()`, because rrefs come in both inbound- and outbound- flavors, and calling `deleteRemoteMapping()` with the wrong flavor would leave half the c-list entry undeleted.
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.
These commits smooth the way for the upcoming GC-over-comms branch. Mostly refactorings, although the
processMaybeFreerearrangement has the potential to slightly modify the behavior (hopefully in a good way).refs #3306