pnnl / lamellar-runtime

Lamellar is an asynchronous tasking runtime for HPC systems developed in RUST
Other
43 stars 5 forks source link

Storing struct in AtomicArray out of bounds error #24

Closed kwaters4 closed 10 months ago

kwaters4 commented 1 year ago

Summary of Problem

When storing a struct into an AtomicArray Lamellar/Rust gives an out of bonds error.

There may be some syntax errors in the code, I can check later if you cannot get it to compile and run.

use lamellar::array::prelude::*;
use serde::{Serialize, Deserilize};
use lamellar::memregion;

#[derive(copy, Clone, Debug, Serialize, Deserialize, Default)]

pub struct Text { 
    pub val : u32;
}

impl Text {
    pub fn new() -> Self {
        self { val: 0}
    }
}

impl Dist for Text {}

fn main() {
    let world = lamellar::LamellarWorldBuilder::new().build();
    let struct_arr = AtomicArray::<Text>::new(&world, 10, Distrubution::Block);
    let usize_arr = AtomicArray::<usize>::new(&world, 10, Distrubtion::Block);

    struct_arr.block_on(
        struct_arr.dist_iter().
        for_each(|elem| elem.store(Text{ val: 24}))
    );
    struct_arr.print();

    usize_arr.print();
    // Store works!
    usize_arr.block_on(usize_arr.store(0,8));
    usize_arr.print();

    let t = Text { val: 42 };
    // Causes panic: index out of bounds for 0 or 5;
    struct_arr.store(5, t);
    struct_arr.print();
}

Rust Version -- 1.67.0 Lamellar Version -- https://github.com/pnnl/lamellar-runtime/tree/dev

rdfriese commented 1 year ago

Encountering the same error, I'll look into it!

rdfriese commented 1 year ago

The main issue here is that I think this code (with the appropriate syntax fixes) should not be allowed to compile but we aren't correctly detecting this from the lamellar side. The runtime does a lot of work underneath the hood when registering structs to be used within a LamellarArray, apparently your derive statement in combination with the impl Dist is enough to satisfy the compiler, when in reality its actually missing a lot of other automatically generated code to make everything else (like element wise operations work properly), I will open another issue so that I can work on detecting this at compile time, in the meantime the following code should run with no issues:

use lamellar::array::prelude::*;
use serde::{Serialize, Deserialize};
// use lamellar::memregion;

// #[derive(Copy, Clone, Debug, Serialize, Deserialize, Default)] //replaced by the lamellar proc macro below
#[lamellar::AmData(Debug, Default, ArrayOps, PartialEq,PartialOrd)]
pub struct Text { 
    pub val : u32
}

impl Text {
    pub fn new() -> Self {
        Self { val: 0}
    }
}

// impl Dist for Text {} //-- automatically derived in #[AmData(ArrayOps)]

fn main() {
    let world = lamellar::LamellarWorldBuilder::new().build();
    let struct_arr = AtomicArray::<Text>::new(&world, 10, Distribution::Block);
    let usize_arr = AtomicArray::<usize>::new(&world, 10, Distribution::Block);

    struct_arr.block_on(
        struct_arr.dist_iter().
        for_each(|elem| elem.store(Text{ val: 24}))
    );
    struct_arr.print();

    usize_arr.print();
    // Store works!
    usize_arr.block_on(usize_arr.store(0,8));
    usize_arr.print();

    let t = Text { val: 42 };
    struct_arr.block_on(struct_arr.store(5, t));
    struct_arr.print();
}

the key things to note is that we are using the #[AmData] proc macro in conjunction with the ArrayOps argument to automatically derive all the necessary traits and backend implementations to use the Text struct in an array.

There is a hard to find page in the docs that talks about this a bit: ArrayOps

We should make this easier to find from the main LamellarArray doc page...

Just as a heads up, in an upcoming update (i.e. the next update), this API will be changing slightly to hopefully make it a little more ergonomic

rdfriese commented 1 year ago

FYI, dev should now contain the fixes that would prevent your original example from compiling. For a bit further reading on changes to the ArrayOps macro, please refer to https://github.com/pnnl/lamellar-runtime/issues/22#issuecomment-1552071079

rdfriese commented 10 months ago

For further info see #25