-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
interruptable eval #1970
base: main
Are you sure you want to change the base?
interruptable eval #1970
Conversation
@@ -263,6 +274,10 @@ array eval_impl(std::vector<array> outputs, bool async) { | |||
return synchronizer; | |||
} | |||
|
|||
void interrupt_eval() { | |||
interrupt_flag() = true; | |||
} |
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.
I think you need a way to clear this if you raced vs the eval. E.g. you didn't hit line 259 above it will still be in true
state on the next call to eval()
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.
Yes good point. There is some subtlety here. I changed it to only set the interrupt flag if an eval is actually ongoing. If it's not ongoing then the flag is not set.
So if you call interrupt before you call eval on the other thread then the eval will keep going. I think this is kind of expected since there is nothing to interrupt and you don't want to interrupt future evals. But it does introduce the possibility of a race. I added a return value to interrupt_eval
so the caller can check if the eval they were expecting to interrupt was actually interrupted.
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.
Seems reasonable
|
||
if (interrupt_flag()) { | ||
interrupt_flag() = false; | ||
synchronizer.attach_event(Event{stream}); |
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.
Does this leave the current work enqueued and cause the eval to return? Or does it drain it (or is that not a thing)?
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.
Work that has already been enqueued will remain so. That includes CPU and GPU work. The GPU command buffers are all committed when this returns.
From https://developer.apple.com/documentation/metal/preparing-your-metal-app-to-run-in-the-background?language=objc we may have to wait for the command buffers to be scheduled.
CC @davidkoski wdyt?