feat(swingset): queue to promise

[mhofman]

closes: #4542
refs: #4318 #5024

Best reviewed commit-by-commit.

Description

This PR updates the processing of message sends onto promises so that:

• The target is checked when plucking the send from the acceptance queue and either queued onto the run-queue or to the corresponding promise queue if the promise is neither resolved nor pipelined
• Sends queued onto promises are moved to the acceptance-queue when their decider changes to a pipelining vat, or when they are resolved (without a target update)

Ultimately the sends should not transit back through the acceptance queue, and should have their target updated right away to release the resolved promise. This will be done in the follow-up PR addressing #5024 (along with immediate queueing of notifies on the run-queue).

Security Considerations

While this change may update the order of message sends between objects and promises, this is valid per our ordering guarantees. In consideration, this is a small ordering update compared to the relaxed ordering we want to introduce.

Documentation Considerations

IOU an update to docs once the queue processing churn has settled

Testing Considerations

Added a test for a preexisting limitation of message sends onto pipelined promises. Update existing tests to account for new message order.

[warner]

Looks pretty good, some cosmetic changes requested, but feel free to land after those.

[warner]

Ok, so messages queued to an object retain a refcount to their target object, but messages queued on a promise do not. Seems reasonable, given the container-ish relationship between promise, queue, and message.

[mhofman]

Ugh? No they do definitely keep a refcount to the target promise. What makes you say that?

[warner]

you deleted the portion of addMessageToPromiseQueue that increments the refcounts on everything in the message

(this comment is supposed to be associated with the deleted lines 825-845 of kernelKeeper.js.. I'm not sure that GitHub renders comments correctly when attached to deleted lines, vs added lines)

yeah now I'm not sure why I said "seems reasonable".. are these refcounts being incremented somewhere else instead now?

[mhofman]

Ha now I understand where the comment comes from.

So with the new structure, a ref count is added when messages are first added to the acceptance queue right after the syscall is received, and decremented when removed from the run-queue just before the delivery. When moved between queues the refcount doesn't change, except when retargeting from a promise to an object, which currently can only happen when moving from the acceptance queue to the run queue. In the next PR retargeting will also possibly happen when moving from the promise queue to the run-queue (currently it goes back to the acceptance queue) when resolving / setting the decider.

I probably should find a place in the code to write that down.

[mhofman]

@warner PTAL with b5d2908

[warner]

Ah, now I see. So previously, while the message was on the run-queue (or the acceptance-queue), it held a refcount on its target+arguments+result (and those refcounts were labeled enq|msg|*). In routeSendEvent, if we determined the message was aimed at an unresolved non-pipelined-vat-decided promise, we used kernelKeeper.addMessageToPromiseQueue() to enqueue it right away, which added refcounts (labelled pq|${kpid}|*). deliverRunQueueEvent called routeSendEvent, then removed the enq|msg|* refcounts. This happens to be "make-before-break", which has a tiny performance improvement (the refcount doesn't drop to zero, so we don't mark it as needing investigation later).

In the branch, routeSendEvent does not do the queue-to-promise itself, it just figures out the target. Upon return, deliverRunQueueEvent has the three-way dispatch (splat, deliver, enqueue-to-promise), and it does the decref for splat and deliver, but doesn't touch the refcounts for enqueue-to-promise.

That reduces churn further (no DB refcount changes for enqueue-to-promise), which is nice. The one downside is that the debug information associated with the refcounts remains labelled as enq|msg|* (i.e. "this refcount comes from the run-queue") rather than naming the particular promise that's holding it. We don't record this information, it just shows up in asserts (and if you enable some debugging messages), but we've considered using it for real (#2870) to help deal with cycles better. If we did that, we'd need to revisit the labelling and make sure it's accurate.

I'm on the fence about that tradeoff: two fewer DB sets during the queue-to-promise step, vs accurate debug tags. I guess the performance argument is stronger. But maybe we should add a note (maybe to incrementRefCount? Or in deliverRunQueueEvent just before the call to addMessageToPromiseQueue?) to remind ourselves that these labels are inaccurate for the sake of performance and reducing DB churn.

[warner]

Oh, and this will show up again if/when we implement redirect/forwarding: if the message is moved to a different queue, the refcount label would get stale. We might need a functional change too: the message will hold a refcount on its original target, but if that was kpid1, and then kpid1 got forwarded to kpid2, we either want to decref kpid1 and incref kpid2, or do some bulk thing (that abandons the idea of tags entirely) and incref kpid2 by the entire refcount of kpid1 (before deleting kpid1 without looking at the refcount).

[mhofman]

Yeah I'd rather find a way that doesn't require updating tags when moving from queue to queue. I see enq as a generic "on some kernel queue". I need to understand the use cases for an actionable queue descriptors, but processing all target, return and slots for every queue switch doesn't seem right.

[warner]

Thanks for the updates.. those help a bunch. Please add one more comment about the refcount debug label becoming stale, and then this is good to land.