Skip to content
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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

sunny-g
Copy link
Contributor

@sunny-g sunny-g commented Aug 24, 2019

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
Copy link
Contributor Author

sunny-g commented May 26, 2021

@scrogson @filmor

@scrogson
Copy link
Member

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
Copy link
Contributor Author

sunny-g commented May 27, 2021

@scrogson fantastic, looking forward to them!

@scrogson
Copy link
Member

scrogson commented Jun 1, 2021

@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
Copy link
Contributor Author

sunny-g commented Jun 1, 2021

@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
Copy link
Member

scrogson commented Jun 1, 2021

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
Copy link
Member

scrogson commented Jun 1, 2021

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
Copy link
Member

scrogson commented Jun 1, 2021

#[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
Copy link
Contributor Author

sunny-g commented Jun 1, 2021

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
Copy link
Member

scrogson commented Jun 1, 2021

  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
Copy link
Contributor Author

sunny-g commented Jun 1, 2021

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
Copy link
Contributor Author

sunny-g commented Jun 2, 2021

@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))
    }
}

@sunny-g sunny-g force-pushed the feat/schedule branch 4 times, most recently from 5dd6de4 to 3911dac Compare June 2, 2021 04:44
@evnu
Copy link
Member

evnu commented Feb 11, 2022

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
Copy link
Contributor Author

sunny-g commented Feb 11, 2022

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

@dimitarvp
Copy link

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

@dimitarvp
Copy link

25 months later -- what's the blocker here? Still sounds like an excellent feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants