schedule_nif improvements

[sunny-g]

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

[sunny-g]

@scrogson @filmor

[scrogson]

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.

[sunny-g]

@scrogson fantastic, looking forward to them!

[scrogson]

@sunny-g here's what I'm thinking...

Simplify the Schedule enum to this...

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 NifResult<Schedule<T>>

[sunny-g]

@scrogson As you might remember, I had that in the original PR.

I changed it because the original solution required your NIF to be Env-aware, which was fine with the original rustler macros since all NIFs had to take in an Env, but seemed messier than necessary when the new macros would do the encoding/decoding to/from Terms for you.

Granted, a pseudo-variadic enum isn't really "clean" either, but that's why it implements From<(..)>/Into<(..)> with default type parameters, so you only need to specify the types and args you use.

I think regardless I should change Schedule::Result to Schedule::Return and the others to Continue.

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.

[scrogson]

As you might remember, I had that in the original PR.

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.

I changed it because the original solution required your NIF to be Env-aware, which was fine with the original rustler macros since all NIFs had to take in an Env, but seemed messier than necessary when the new macros would do the encoding/decoding to/from Terms for you.

With the new syntax, some things still need an Env anyways, so I don't see any issue with this solution requiring Env in order to work (which reminds me...my reschedule! macro sketch was missing the Env and scheduler flag).

[scrogson]

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)

[scrogson]

#[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)
    }
}

[sunny-g]

2 things:

1. the last comment looks fine, but one thing to note: are the flags required, or are you specifying it to just highlight that they might differ from the NIF macro definition? If so, the enum would still need work.
2. can you help me understand the issue you're having with the PR as it is currently? Ergonomics, maintainability, extensibility, or something else? I was attempting something that in the best case would be the minimally unintuitive, and in the worst case, would be verbose but still rust-y.

[scrogson]

1. are the flags required, or are you specifying it to just highlight that they might differ from the NIF macro definition?

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.

If so, the enum would still need work.

Yes, I think we just add the schedule flag in the 3rd position in the Continue variant.

can you help me understand the issue you're having with the PR as it is currently?

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 reschedule function at compile-time for each NIF?

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?)

[sunny-g]

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.

Makes sense.

Yes, I think we just add the schedule flag in the 3rd position in the Continue variant.

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.

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.

That's understandable, but is also the reason I added the Into conversion, so that you wouldn't have to worry about it at all.

I'm now wondering if an elegant solution could be to generate a reschedule function at compile-time for each NIF?

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?)

This is actually the approach I started with on this pass, but had problems with it because inside the function, the Ident of the function's name is scoped to the function itself, not the NIF type that gets created - meaning that inside an implementation of scheduled_fac, scheduled_fac::reschedule would refer to that function, not the generated NIF struct. This could be avoided if crate::Nif was implemented on the defined function itself, and I'm not sure what's preventing that.

I'll try again anyways, and regardless will also add a macro so that you don't have to use any variants.

[sunny-g]

@scrogson so I wasn't able to work around the scoping issue, but have now added a reschedule macro that I think gets us the best of both worlds in terms of terseness and ergonomics:

#[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))
    }
}

[evnu]

Can we go forward with this PR? I think convenient rescheduling would be very nice to have. What is missing to bring this forward?

[sunny-g]

@evnu Lemme take another look at this, I might be able to improve the API a bit. Otherwise it works.

[dimitarvp]

Sorry to bother -- does anyone has the time to tackle this? It's a nice feature.