-
Notifications
You must be signed in to change notification settings - Fork 628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DRAFT] Add async command framework using Java 21 language features #6518
base: main
Are you sure you want to change the base?
Conversation
We've discussed putting a command-based rewrite in its own package instead of wpilibjN. For example, |
It's too early to be bikeshedding 😄 My goal here is to inform future command framework changes, not to have a ready-to-go implementation. (Though I do agree that |
Any reason for |
It's clearer about what it does without needing to assume a baseline knowledge of multithreading concepts. And it's apt if async functions/continuations/coroutines (or wherever else you'd like to call them) are taught as pausable functions |
I strongly disagree that "pausable functions" is the term that should be used -- it's not a term that is commonly used and will likely cause confusion. Therefore I maintain my suggestion to name it Command-based historically has a problem of being very DSL-y and magic incantations; this is a good opportunity to take a big step to using common industry terms. Let's take that step. |
What happens if AsyncCommand.pause() is called from outside an AsyncCommand virtual thread? |
With the current impl, it would suspend the entire carrier thread. Throwing could be better. |
This seems like it could also support differing command interruption semantics in addition to priorities. If we have both of those features, we can very naturally represent "default commands" as a minimum-priority command with suspend or reschedule behavior on interruption. |
This is a good argument. I'll note that virtual thread pausing does not behave like
As written, it would just block the calling thread. Yotam's suggestion of throwing would certainly be better, since it makes no sense to pause outside a command.
I like this idea. How would a resume-after-interruption command look in user code? |
I think we should re-purpose the So, it'd look like: run(...).onInterrupt(InterruptBehavior.kSuspend);
run(...).onInterrupt(() -> { ... }, InterruptBehavior.kSuspend); Suspended commands will be re-scheduled as soon as their requirements become available. The implementation that allows this could also allow us to eventually support queued commands. |
Reminder that commands are interrupted by interrupting their vthread, which triggers an Maybe we could have a This is not a complete and correct implementation, but for the sake of example: // AsyncScheduler.java
public void pause(AsyncCommand command, Measure<Time> time) {
long ms = (long) time.in(Milliseconds);
boolean suspendOnInterrupt = command.interruptBehavior() == kSuspend;
do {
try {
Thread.sleep(ms);
} catch (InterruptedException e) {
if (suspendOnInterrupt)
continue;
throw e;
}
} while (command.priority() < getHighestPriorityCommandUsingAnyOf(command.requirements()).priority());
} |
This is very similar syntax to my python coroutine command impl. encoders.reset()
while getDistance() < distance:
yield
tankDrive(1, 1)
stop() It does active cooperation with yield. I've been mulling over doing yield based coroutines vs async/await based coroutines throughout wpilib and I think there needs to be more exploring do on which is better long-term (idk if java has async/await keywords incoming but it would be nice if the python/java apis were similar). |
Java isn't getting async/await keywords, the best we have is Futures (more or less analogous to JavaScript promises). That's error prone since it's totally valid to call a method that returns a Future and then not await its result. A yield-based API seems less prone to unintended behavior |
int ns = (int) (time.in(Nanoseconds) % 1e6); | ||
|
||
// Always sleep once | ||
Thread.sleep(ms, ns); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be waiting for 20ms - execution time so it resumes at a 20ms clock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Though the underlying thread jitter tends to make this moot, where the time spent parked can be off by several milliseconds. Worst I've seen is a request for 20ms actually sleep for 36... That might get improved with optimizations to reduce the number of locking calls, or maybe it just ran afoul of a stop-the-world GC pass
3661407
to
d570544
Compare
Virtual threads ended up being too prone to deadlocking and inconsistent timing, since it's at the mercy of the JVM's thread scheduler for when parked vthreads are resumed. I was consistently seeing sleep times of 20ms end up taking ~25ms, with the worst outlier at ~38ms. I've pushed changes using Java's AsyncCommand cmd = someResource.run(() -> {
var timer = new Timer();
timer.start();
while (!timer.hasElapsed(5.000)) {
AsyncCommand.yield();
doSomething();
}
}).named("Demo"); One potential issue here is that there's no way to run cleanup logic on command interrupts. Continuations will just be kicked out of the scheduler if their command is canceled. |
…package Remove resources-as-executors
Track all running nested commands, not just top-level Allows checking for any running command and awaiting any nested command
Incomplete implementation; breaks when a command interrupts a suspendable command of the same priority Add a standard unit for nanoseconds to support sub-millisecond pause precision
…ame thread as commands
Allows command groups and compositions to reuse logic
Reframe default commands as suspendable minimum-priority commands
Ther permits the use of commands to control a subsystem comprised of two separate mechanisms that need to be controlled together (eg a pink arm) that would traditionally need to be controlled without using commands to avoid scenarios in which one mechanism is controlled and the other is not
Scheduler now pulls the default command from resources, instead of keeping track internally
Bump Java to 21.0.2 due to compiler bug in 21.0.1
Move examples to "coroutine" package instead of "async"
Helpful for timed sequences without needing to wrap async methods in a command
Were referencing global state Move to trigger class (which was the only place that used them anyway) TODO: Refactor triggers to be generic so logic doesn't need to be duplicated between v2 and v3
Child commands do not need to require the same resources as their parent Sibling commands, however, cannot conflict
Rather than just pushing them to be on deck, to be picked up in the next scheduler cycle Removes the delay between scheduling a deeply nested composition and the when lowest-level commands that actually /do/ things begin to run
1321fd4
to
19198c4
Compare
To allow for recovery to a safe state (eg turning off motors) when no default command is available for its required resources This does NOT run when interrupted by another command using the same resource, with the reasoning that the incoming command will do something with the resource
19198c4
to
1d70f6d
Compare
Tweak docs a bit
Since it's not just for buttons
The framework fundamentally relies on the continuation API added in Java 21 (which is currently internal to the JDK). Continuations allow for call stacks to be saved to the heap and resumed later. This cannot run on the roborio due to continuations not being implemented for 32-bit ARM platforms in OpenJDK. The 2027 MRC will use a CPU with a 64-bit ARM architecture, which is supported by continuations.
The async framework allows command bodies to be written in an imperative style:
Which would look something like this using the current functional framework:
Commands could be written as thin wrappers around regular imperative methods:
However, an async command will need to be actively cooperative and periodically call
coroutine.yield()
in loops to yield control back to the command scheduler to let it process other commands.There are also some other additions like priority levels (as opposed to a blanket yes/no for ignoring incoming commands), factories requiring names be provided for commands, and the scheduler tracking all running commands and not just the highest-level groups. However, those changes aren't unique to an async framework, and could just as easily be used in a traditional command framework.
Links:
JEP 425 - Virtual Threads
JEP 453 - Structured Concurrency
The async version of the rapid react example project
Scheduler.java
ParallelGroup.java
Sequence.java