Profiler/instrumentation support and promise jobs #34
Conversation
Rather than treating all unknown commands as a request for the worker to silently exit (rc=0), we make a specific 'q' command for that purpose. Unknown commands now cause an error exit (rc=2), with a message (to stderr) about what the surprising command was. This includes the characters that are only used in downstream responses to upstream `issueCommand()` calls, which might help diagnose desynchronization errors faster.
Co-authored-by: Mathieu Hofman <[email protected]>
exit with error upon unknown commands
commit 'aa494cee6f6b5cda70f7c685535940dae4321252'
Drain promise queue after host job, then run finalizers Repeat if needed
mhofman
force-pushed
the
mhofman/6759-update-xs
branch
from
ccc51a6 to
2a42752
Compare
|
Alright so apparently Speaking of which, some checks in the code do Btw, leaving To avoid churn I forced pushed the changes: ccc51a6..2a42752 |
|
Ugh apparently this repo requires approving reviews now before pushing to |
|
Let's see, we want agoric-sdk |
|
Please review then :) |
| void fxRunLoop(txMachine* the) | ||
| { | ||
| c_timeval tv; | ||
| txNumber when; | ||
| txJob* job; | ||
| txJob** address; | ||
| fxEndJob(the); |
There was a problem hiding this comment.
This looks like a potential behavioral change, but I can't remember why we needed this here. I think you added it in 84aa01a , but the comment just says "Platform runloop tweaks, Snapshot dump tweaks".
I think the command we send down to the worker gets its first turn of execution before we call fxRunLoop(). I think fxEndJob() basically does finalization callbacks and fxCheckUnhandledRejections. We probably don't do a lot during that first turn (https://github.com/Agoric/agoric-sdk/blob/2529e82427277d98ea777c5a35e6bd753e65e6f7/packages/SwingSet/src/supervisors/supervisor-helper.js#L32 does a deliberate stall, which would consume the first turn), but it's conceivable that this might trigger organic GC, and then some finalizers could become ready to run.
I'm ok with this change as long as you can't think of any reason why we need to maintain the previous behavior.
There was a problem hiding this comment.
The previous behavior was "not expected" and buggy.
If the command job enqueued promises, it would not drain those, but instead run endJob first. Only at that point it would drain promise queues (for both jobs)
The expectation is that reactions form an I/O job are processed immediately, then come the "gc" jobs (and its reactions).
The previous addition was necessary because if the main command job didn't queue promises, endJob would not trigger. It was actually the wrong fix because the same thing would happen if a subsequent I/O job (e.g. setImmediate) didn't queue promises, it wouldn't run endJob either.
Now the logic does the expected thing:
- Run the I/O job (command or timers), and drain promise reactions
- Run GC jobs and check for unhandled errors, and drain promise reactions
- Repeat
This is the change included in Agoric/agoric-sdk#6920
When approved I plan on force pushing to master to avoid dealing with churning the upstream PR. Leaving this PR as "draft to indicate it won't me merged through GitHub)
I tried to cleanly merge @phoddie's fork, but there seemed to have been a lot of merge conflicts churn. Hopefully an actual merge commit will allow future syncs to succeed more easily. The merge with the most conflict resolution is a0627a8. I recommend reviewing with a tool that allows correctly visualizing merge commit and branching.
The instrumentation changes were disabled as I wasn't confident the
fxIsConnectedreturning 1 unconditionally didn't have a side effect since it seemsfxParseScripthas an extra call in that case.I also included a tweak to the run loop to process promise jobs according to a discussion I had with @phoddie and @patrick-soquet.
This also includes all the fixes of the psm branch which are already included on agoric-sdk master.