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

Full Altrep Support #250

Merged
merged 21 commits into from
Aug 15, 2021
Merged

Full Altrep Support #250

merged 21 commits into from
Aug 15, 2021

Conversation

andy-thomason
Copy link
Contributor

This PR adds an "Altrep" wrapper and the beginnings of the means to implement new Altrep classes.

@clauswilke
Copy link
Member

Super, thanks! Could you add some comments that explain what the various API functions do?

fn serialized_state(&self) -> Robj {
().into()
}

/// Duplicate this object, possibly duplicating attributes.
fn duplicate_ex(&self, _deep: bool) -> Robj {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "ex" stand for? "extended"? Can you think of a different choice that is a little more intuitive?

@andy-thomason
Copy link
Contributor Author

I'm not sure yet how we will define new Altrep classes.

Given a trait like the ones described, we should be able to make a "universal adapter" from the Altrep
callbacks to our trait methods.

To test, I'll implement From<Range<i32>> as an altrep class.

@@ -422,6 +424,37 @@ pub enum RType {
Unknown,
}

pub fn rtype_to_sxp(rtype: RType) -> i32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a brief, 1-2 sentence documentation would be helpful here also.

@andy-thomason andy-thomason marked this pull request as draft August 6, 2021 20:22
@andy-thomason
Copy link
Contributor Author

andy-thomason commented Aug 6, 2021

I'm probably going to narrow the definition of Altrep to make it easier to use.

Altrep objects have

  • A class object (a raw object wrapping the vtable)
  • A state object
  • An optional materialized vector

If we always use a struct for the state object (ie. Extptr) we can implement
the altrep functions as a methods of the state.

In an ideal world, we would just need to implement the "elt" method for an
altrep object and leave all the rest to Rust.

The state object should be a regular Rust object rather than an R object.

    struct MyCompactIntRange {
        start: i32,
        length: i32,
        step: i32,
    }

    impl AltIntegerImpl for MyCompactIntRange {
        fn elt(&self, index: usize) -> i32 {
            self.start + self.step * index
        }
    }

    let mystate = MyCompactIntRange { start: 1, length: 10, step: 1 };
    let myobj = Altrep::new(mystate);

@yutannihilation
Copy link
Contributor

ALTREP_LENGTH might have gone in the recent R API change.

   error[E0425]: cannot find function, tuple struct or tuple variant `ALTREP_LENGTH` in this scope
    --> extendr-api/src/wrapper/altrep.rs:83:27
     |
  83 |                 let len = ALTREP_LENGTH(x);
     |                           ^^^^^^^^^^^^^ not found in this scope
  
  error: aborting due to previous error

@andy-thomason
Copy link
Contributor Author

@yutannihilation did we establish a mechanism for using #[cfg] with libR-sys?

If so, I can screen out some of the newer features.

The ALTREP_ functions are good shortcuts to calling altrep dispatches.

@yutannihilation
Copy link
Contributor

did we establish a mechanism for using #[cfg] with libR-sys?

(Sorry, I forgot to answer...) Yes, for internal usages within extendr crates, we can set cfg in build.rs.

let major = env::var("DEP_R_R_VERSION_MAJOR").unwrap();
let minor = env::var("DEP_R_R_VERSION_MINOR").unwrap();
// let patch = env::var("DEP_R_R_VERSION_PATCH").unwrap();
// R_NewEnv is available as of R 4.1.0
if &*major >= "4" && &*minor >= "1" {
println!("cargo:rustc-cfg=use_r_newenv");
}

If so, I can screen out some of the newer features.

In case a feature is not available in the older versions of R, it sounds the right thing to do. But, this time, the case is that it's not available on the newer version, which means we'll need to remove it in future when the current versions are out of the range of our minimum supported version. So, I'm wondering if it's worth the effort...

@clauswilke
Copy link
Member

But, this time, the case is that it's not available on the newer version, which means we'll need to remove it in future when the current versions are out of the range of our minimum supported version. So, I'm wondering if it's worth the effort...

Yeah, I had the same thoughts. If it's a feature that is going away we should just work without for all R versions. Or, if critical, reach out to the R core developers and ask them to not remove the feature.

@yutannihilation
Copy link
Contributor

For ALTREP_LENGTH(), it seems we can use XLENGTH_EX() instead.

https://github.com/wch/r-source/blob/ea1c48271846cd02ad5f93b5562152ad82778d8d/src/include/Rinlinedfuns.h#L202-L205

@yutannihilation
Copy link
Contributor

Or just Rf_xlength()? Sorry, I'm not familiar with the things around here...

@andy-thomason andy-thomason linked an issue Aug 12, 2021 that may be closed by this pull request
@andy-thomason
Copy link
Contributor Author

Claus is right that we can't rely on the functions now in defn.h

The sum, min and max functions should work for code, so we can always
call them using call!

I still have to implement defaults duplicate and coerce, but then we can merge.

I am planning to finish off the last few SXP types as part of the grant contract.

@andy-thomason andy-thomason changed the title Basic Altrep Support Full Altrep Support Aug 15, 2021
@andy-thomason andy-thomason marked this pull request as ready for review August 15, 2021 23:41
@andy-thomason andy-thomason merged commit 1d85fc2 into master Aug 15, 2021
@andy-thomason andy-thomason deleted the basic-altrep-support branch August 15, 2021 23:41
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.

Full ALTREP support
3 participants