feat(cosmic-swingset): Depth first scheduling #6221
Conversation
|
@warner I've re-arranged in your WIP commits into singular ones (best reviewed independently), and tweaked a few things, in particular:
Here is a rough diff (with base shift clutter) of what changed: https://github.com/Agoric/agoric-sdk/compare/b0e606850..6210-depth-first-scheduling |
mhofman
force-pushed
the
6210-depth-first-scheduling
branch
from
c52d25d to
9cf6302
Compare
| // Process our begin, queued actions, and end. | ||
| await processAction(beginBlockAction); // BEGIN_BLOCK | ||
| // First we allow the timer to poll, which might push work onto the | ||
| // kernel run-queue, and gets the first cycle. |
There was a problem hiding this comment.
This comment, in this commit ("disentangle launch-chain some more") and the next, is not accurate: timer work gets pushed onto the end of the run-queue, never getting the first cycle. Let's change it to:
We update the timer device at the start of each block, which might push work onto the end of the kernel run-queue (if any timers were ready to wake), where it will be followed by actions triggered by the block's swingset transactions.
Then, in the commit where we switch to cycles, add:
If the queue was empty, the timer work gets the first "cycle", and might run to completion before the block actions get their own cycles.
| // and disables commit/abort. | ||
|
|
||
| const inboundQueuePrefix = getHostKey('inboundQueue.'); | ||
| const inboundStorage = harden({ |
There was a problem hiding this comment.
let's name this inboundQueueStorage
| return val && JSON.parse(val); | ||
| }, | ||
| set: (key, value) => | ||
| value === undefined |
There was a problem hiding this comment.
Does the makeQueue code ever do set(key, undefined)? I thought it does explicit delete.
The JSON serialization surprises me, but I guess the old version was relying upon the call send-to-cosmos function to serialize the value along with the rest of the request?
There was a problem hiding this comment.
I don't know what values are passed in, and I need to handle undefined somehow. I think deleting is just as correct as writing ''.
Yeah I dug through and some layer other than the queue was doing serialization before
There was a problem hiding this comment.
The kvStore can (well, is supposed to) be able to store empty strings just fine. set(key, '') is entirely different than delete(key).
It might be the case that set(key, undefined) is the same as delete(key). I wouldn't like that in a swingstore API, but I suspect at least one of our storage layers behaves that way.
Do you know what was trying to store undefined? I know the Actions are objects, and for sure that needs to be serialized into the swingstore's key: string, value: string API, but I'd be surprised for any of the Actions to be an undefined.
| blockHeight, | ||
| runNum, | ||
| }); | ||
| await bootstrapBlock(); |
There was a problem hiding this comment.
The last commit ("Depth first scheduling") changes this line from await processAction(action.type, bootstrapBlock) to await bootstrapBlock(). Should that change have happened in an earlier commit? It seems to me that this specific commit should only be adding the cosmic-swingset-run-start logs around a call that should have been the same as in the previous commit. (The end result might be the same, and seems correct, but the path by which the commits get there might be slightly off).
There was a problem hiding this comment.
Good catch, it should not have happened at all. I'm using processAction to console.log any error. The runTime measurement in this case are simply dropped.
Remove unused events
The following `blockingSend()` events are now logged when cosmos invokes cosmic-swingset, rather than later (as the action queue is processed, within the END_BLOCK phase): * cosmic-swingset-bootstrap-block-start * cosmic-swingset-bootstrap-block-finish * cosmic-swingset-begin-block * cosmic-swingset-end-block-start * cosmic-swingset-end-block-finish * cosmic-swingset-commit-block-start * cosmic-swingset-commit-block-finish
mhofman
force-pushed
the
6210-depth-first-scheduling
branch
from
9cf6302 to
c6170a7
Compare
mhofman
added
the
automerge:no-update
closes: #6210
Description
This PR further refactors cosmic-swingset and add an
inboundQueuewhich buffers the cosmos inbound actions on the JS side, processing them one by one by running swingset to quiescence after delivering each action.Security Considerations
This approach is not safe against infinite message loops, which would cause a Denial Of Service. However the current contracts are trusted to not behave like that.
Testing Considerations
Tested locally with the loadgen. Load testing in progress on ollinet.