-
Notifications
You must be signed in to change notification settings - Fork 225
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
schedule_nif improvements #232
base: master
Are you sure you want to change the base?
Conversation
aad8c2f
to
f7e3952
Compare
Hey @sunny-g! Thanks for updating this. I've got some ideas here that I want to share but I don't have the time to articulate it thoroughly right now. I'll take some time over the next couple days to put a review together. |
@scrogson fantastic, looking forward to them! |
@sunny-g here's what I'm thinking... Simplify the pub enum Schedule<T> {
Return(T),
Continue(Nif, Vec<Term>),
}
impl<T> NifReturnable for Schedule<T> where T: NifReturnable {
...
} Then create a macro which builds the correct data structure: reschedule!(my_nif, a, b, c, ...); This should allow NIFs to return |
@scrogson As you might remember, I had that in the original PR. I changed it because the original solution required your NIF to be Granted, a pseudo-variadic I think regardless I should change Let me know if this explanation changes anything - if not at all, I'll go with what you suggested. Alternatively, I could split the difference and add a similar macro to what you've suggested that's more ergonomically variadic. |
I went back and looked at the original commits as I didn't remember what it looked like before, and yes it is quite similar to my suggestion.
With the new syntax, some things still need an |
Another thought here is maybe we only need a macro or some function to convert the args? And use: let args: Vec<Term> = args!(env, a, b, c, ...);
Schedule::Continue(my_nif, args, DirtyCpu) |
#[rustler::nif]
fn scheduled_fac(env: Env, input: u32, result: Option<u32>) -> Schedule<u32> {
let result = result.unwrap_or(1);
if input == 0 {
Schedule::Return(result)
} else {
Schedule::Continue(scheduled_fac, args!(env, input - 1, Some(result * input)), DirtyCpu)
}
} |
2 things:
|
The schedule flag should be allowed to override the original NIF when needed. I personally haven't found a direct need for this, but I have heard of some instances where depending on the input length you may not need to run something on the dirty schedulers and not pay the extra cost.
Yes, I think we just add the schedule flag in the 3rd position in the
The 2 things that I'm not happy with are the return type and the various functions to represent different arity. It feels a bit unintuitive to me. I'm now wondering if an elegant solution could be to generate a scheduled_fac::reschedule(env, input - 1, Some(result * input), DirtyCpu) This would probably allow us to ensure type safety on what's passed in (which I think is part of the why behind your current implementation?) |
Makes sense.
I meant in the sense that it couldn't be "optional", so the alternative would requiring that you pass a flag, even if it's normal, which maybe isn't that big of a deal.
That's understandable, but is also the reason I added the
This is actually the approach I started with on this pass, but had problems with it because inside the function, the I'll try again anyways, and regardless will also add a macro so that you don't have to use any variants. |
@scrogson so I wasn't able to work around the scoping issue, but have now added a #[rustler::nif]
fn scheduled_fac<'a>(input: u32, result: Option<u32>) -> Schedule<scheduled_fac, u32, u32, u32> {
let result = result.unwrap_or(1);
if input == 0 {
Schedule::Return(result)
} else {
reschedule!(SchedulerFlags::Normal, input - 1, Some(result * input))
}
} |
5dd6de4
to
3911dac
Compare
Can we go forward with this PR? I think convenient rescheduling would be very nice to have. What is missing to bring this forward? |
@evnu Lemme take another look at this, I might be able to improve the API a bit. Otherwise it works. |
Sorry to bother -- does anyone has the time to tackle this? It's a nice feature. |
b3d5278
to
9b86803
Compare
9b86803
to
5addb9f
Compare
5addb9f
to
c02e991
Compare
25 months later -- what's the blocker here? Still sounds like an excellent feature. |
Re-implemented for the new
#[rustler::nif]
API (excellent work btw).Let me know what y'all think! Also open to renaming things if y'all think they make more sense.
Previous discussion: #106 #107