Skip to content

schedule_nif improvements#232

Open
sunny-g wants to merge 11 commits into
rusterlium:masterfrom
sunny-g:feat/schedule
Open

schedule_nif improvements#232
sunny-g wants to merge 11 commits into
rusterlium:masterfrom
sunny-g:feat/schedule

Conversation

@sunny-g

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

Copy link
Copy Markdown
Contributor

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

sunny-g commented May 26, 2021

Copy link
Copy Markdown
Contributor Author

@scrogson @filmor

@scrogson

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

sunny-g commented May 27, 2021

Copy link
Copy Markdown
Contributor Author

@scrogson fantastic, looking forward to them!

@scrogson

scrogson commented Jun 1, 2021

Copy link
Copy Markdown
Member

@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

sunny-g commented Jun 1, 2021

Copy link
Copy Markdown
Contributor Author

@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

scrogson commented Jun 1, 2021

Copy link
Copy Markdown
Member

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

scrogson commented Jun 1, 2021

Copy link
Copy Markdown
Member

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

scrogson commented Jun 1, 2021

Copy link
Copy Markdown
Member
#[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

sunny-g commented Jun 1, 2021

Copy link
Copy Markdown
Contributor Author

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

scrogson commented Jun 1, 2021

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

sunny-g commented Jun 1, 2021

Copy link
Copy Markdown
Contributor Author

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

sunny-g commented Jun 2, 2021

Copy link
Copy Markdown
Contributor Author

@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

evnu commented Feb 11, 2022

Copy link
Copy Markdown
Member

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

sunny-g commented Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

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

@dimitarvp

Copy link
Copy Markdown

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

@dimitarvp

Copy link
Copy Markdown

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